Re: [pkg-discuss] Code Review: mDNS mirror detection
On 05/17/10 06:58 PM, johan...@opensolaris.org wrote: On Mon, May 17, 2010 at 05:02:10PM -0500, Shawn Walker wrote: On 05/17/10 04:24 PM, johan...@opensolaris.org wrote: On Mon, May 17, 2010 at 03:40:18PM -0500, Shawn Walker wrote: On 05/17/10 03:22 PM, johan...@opensolaris.org wrote: ... I've incorporated these changes into the latest webrev, which is available below. Hopefully, this addresses the remainder of your concerns. http://cr.opensolaris.org/~johansen/webrev-6957-3/ http://cr.opensolaris.org/~johansen/webrev-6957-4/ Here's a webrev with the last set of changes that you requested. Awesome! Thanks for going the extra mile. Ship it, -Shawn ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On Mon, May 17, 2010 at 05:02:10PM -0500, Shawn Walker wrote: > On 05/17/10 04:24 PM, johan...@opensolaris.org wrote: > >On Mon, May 17, 2010 at 03:40:18PM -0500, Shawn Walker wrote: > >>On 05/17/10 03:22 PM, johan...@opensolaris.org wrote: > >>... > >>>I've incorporated these changes into the latest webrev, which is > >>>available below. Hopefully, this addresses the remainder of your > >>>concerns. > >>> > >>> http://cr.opensolaris.org/~johansen/webrev-6957-3/ http://cr.opensolaris.org/~johansen/webrev-6957-4/ Here's a webrev with the last set of changes that you requested. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On 05/17/10 04:24 PM, johan...@opensolaris.org wrote: On Mon, May 17, 2010 at 03:40:18PM -0500, Shawn Walker wrote: On 05/17/10 03:22 PM, johan...@opensolaris.org wrote: ... I've incorporated these changes into the latest webrev, which is available below. Hopefully, this addresses the remainder of your concerns. http://cr.opensolaris.org/~johansen/webrev-6957-3/ src/depot.py: line 552: nit: s/used/provided/ ? trying to figure out some way to make it not sound like you can only use -d or --file-root (as opposed to both) Changed to, "At least one of PKG_REPO, -d, or --file-root must be provided" src/man/pkg.depotd.1m.txt: What mdns or dns-sd means is never defined here. Also, should it be written as mDNS instead of "mdns" ? Also, is there another man page for see also that might be helpful with this? I've made the corrections that Danek suggested; however, I'm not sure what you'd like me to do about defining mdns or dns-sd. Multicast DNS and DNS Service-Discovery or do you want me to quote from the RFCs for these things? What Danek said is what I was trying to :) So nothing more needed... src/modules/server/depot.py: line 147: To avoid affecting the class variable, I'd insert a line above this with: "self.ops_list = self.ops_list[:]". How about "self.ops_list = self.REPO_OPS_MIRROR[:]" on line 145 instead? Sure. src/modules/server/repository.py: lines 427-429: I think this should be above the self.__reset() block These line numbers don't match anything that I changed in repository.py. Can you clarify this comment? Err...sorry, that should have been repositoryconfig.py. Cheers, -Shawn ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On Mon, May 17, 2010 at 03:40:18PM -0500, Shawn Walker wrote: > On 05/17/10 03:22 PM, johan...@opensolaris.org wrote: > ... > >I've incorporated these changes into the latest webrev, which is > >available below. Hopefully, this addresses the remainder of your > >concerns. > > > > http://cr.opensolaris.org/~johansen/webrev-6957-3/ > > src/depot.py: > line 552: nit: s/used/provided/ ? trying to figure out some way > to make it not sound like you can only use -d or --file-root > (as opposed to both) Changed to, "At least one of PKG_REPO, -d, or --file-root must be provided" > src/man/pkg.depotd.1m.txt: > What mdns or dns-sd means is never defined here. Also, should it > be written as mDNS instead of "mdns" ? Also, is there another > man page for see also that might be helpful with this? I've made the corrections that Danek suggested; however, I'm not sure what you'd like me to do about defining mdns or dns-sd. Multicast DNS and DNS Service-Discovery or do you want me to quote from the RFCs for these things? > src/modules/server/depot.py: > line 147: To avoid affecting the class variable, I'd insert > a line above this with: "self.ops_list = self.ops_list[:]". How about "self.ops_list = self.REPO_OPS_MIRROR[:]" on line 145 instead? > src/modules/server/repository.py: > lines 427-429: I think this should be above the self.__reset() block These line numbers don't match anything that I changed in repository.py. Can you clarify this comment? > src/tests: > The only thing you're missing now is a test in cli.t_pkg_depotd to > verify that you can start a depot server with just a file_root and > that everything but /file/ requests are disabled. I'll see what I can do. Thanks, -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
Shawn Walker wrote: > src/man/pkg.depotd.1m.txt: > What mdns or dns-sd means is never defined here. Also, should it > be written as mDNS instead of "mdns" ? Also, is there another > man page for see also that might be helpful with this? mdnsd(1M) and dns-sd(1M). The former appears to still not be available in OpenSolaris, but we should probably reference it anyway. It should be spelled "mDNS", and probably referred to the first time by its expansion, as in the synopsis of dns-sd(1M). Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On 05/17/10 03:22 PM, johan...@opensolaris.org wrote: ... I've incorporated these changes into the latest webrev, which is available below. Hopefully, this addresses the remainder of your concerns. http://cr.opensolaris.org/~johansen/webrev-6957-3/ src/depot.py: line 552: nit: s/used/provided/ ? trying to figure out some way to make it not sound like you can only use -d or --file-root (as opposed to both) src/man/pkg.depotd.1m.txt: What mdns or dns-sd means is never defined here. Also, should it be written as mDNS instead of "mdns" ? Also, is there another man page for see also that might be helpful with this? src/modules/server/depot.py: line 147: To avoid affecting the class variable, I'd insert a line above this with: "self.ops_list = self.ops_list[:]". src/modules/server/repository.py: lines 427-429: I think this should be above the self.__reset() block src/tests: The only thing you're missing now is a test in cli.t_pkg_depotd to verify that you can start a depot server with just a file_root and that everything but /file/ requests are disabled. Otherwise, I'm much happier with this set of changes now. This makes the repository/depot a lot more robust. Cheers, -Shawn ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On Thu, May 13, 2010 at 08:10:17PM -0500, Shawn Walker wrote: > If this is what you're really wanting, what I'd suggest is that you > change pkg.depotd such that: > > * --file-root or -d is required (but both may be specified) > > * ensure that repo_dir is "" for your particular use case (where no > configuration is present) > > * change repositoryconfig.py logic in is_valid_property_value to not > require publisher prefix (as it already does for alias) > > * change RepositoryConfig to not require a pathname, which makes > operations like __read return after __reset(), and write() simply > becomes a no-op > > * change repository.py to simply generate a default repositoryconfig > in the event no repo_root exists > > * change Repository to account for repo_root being None. To do this, > I'd add a RepositoryUnsupportedOperationError exception, and then > change functions like catalog_0 and catalog_1, etc. to have a check > like: > > if not self.catalog_root: > raise RepositoryUnsupportedOperationError() > >...obviously other things will have to change too, but you get the >idea. I've incorporated these changes into the latest webrev, which is available below. Hopefully, this addresses the remainder of your concerns. http://cr.opensolaris.org/~johansen/webrev-6957-3/ -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On 05/13/10 07:44 PM, johan...@opensolaris.org wrote: On Thu, May 13, 2010 at 07:39:55PM -0500, Shawn Walker wrote: How would it find the configuration file used by the standard depot if it's mirroring /var/pkg/download? The only way that would happen is if you set the repo_dir to the same location as the depot. Why would you do that? I don't set repo_dir, because the mDNS mirror doesn't need anything out of that directory. In that case, it inherits its default, which is only a problem if we're required to have a cfg_cache. What does "inherits its default" mean? I assume you're referring to SMF. If so, see below. Until I understand the above, this still doesn't make any sense to me. Remember that we had a conversation about a month ago about turning the depot into a more modular solution for a variety of pkg(5) problems. In this case, try not to think of the depot as a depot, but just the functionality that we need to implement mirroring a /var/pkg/download directory, and advertising ourselves over mDNS. As I understand it, you're wanting to stand up a pkg.depotd with nothing more than a directory that contains package files laid out according to file_manager's usual scheme. That is, you only have a file_root; no cfg_cache no repo_dir, and none of the other roots technically exist. If you have only have a file_root, it becomes very difficult to determine validity of the target. Really, you don't even have a repository at this point, at least, not a complete one. If this is what you're really wanting, what I'd suggest is that you change pkg.depotd such that: * --file-root or -d is required (but both may be specified) * ensure that repo_dir is "" for your particular use case (where no configuration is present) * change repositoryconfig.py logic in is_valid_property_value to not require publisher prefix (as it already does for alias) * change RepositoryConfig to not require a pathname, which makes operations like __read return after __reset(), and write() simply becomes a no-op * change repository.py to simply generate a default repositoryconfig in the event no repo_root exists * change Repository to account for repo_root being None. To do this, I'd add a RepositoryUnsupportedOperationError exception, and then change functions like catalog_0 and catalog_1, etc. to have a check like: if not self.catalog_root: raise RepositoryUnsupportedOperationError() ...obviously other things will have to change too, but you get the idea. The above is the most flexible solution long term. I don't know yet how to reconcile the fact that a file directory alone doesn't constitute a valid repository. If we say that it is, then I need to account for that in the on-disk format proposal. The easiest way to do unit tests for the above is going to be to use the Repository class directory in one of the api test suites. I believe this resolves your concern about picking up an unwanted cfg_cache, requiring one to actually run, and my concerns about not using what's already there for administrators that have the right configuration. Cheers, -Shawn ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On Thu, May 13, 2010 at 07:39:55PM -0500, Shawn Walker wrote: > How would it find the configuration file used by the standard depot > if it's mirroring /var/pkg/download? The only way that would happen > is if you set the repo_dir to the same location as the depot. Why > would you do that? I don't set repo_dir, because the mDNS mirror doesn't need anything out of that directory. In that case, it inherits its default, which is only a problem if we're required to have a cfg_cache. > Until I understand the above, this still doesn't make any sense to me. Remember that we had a conversation about a month ago about turning the depot into a more modular solution for a variety of pkg(5) problems. In this case, try not to think of the depot as a depot, but just the functionality that we need to implement mirroring a /var/pkg/download directory, and advertising ourselves over mDNS. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On 05/13/10 07:08 PM, johan...@opensolaris.org wrote: On Thu, May 13, 2010 at 06:53:28PM -0500, Shawn Walker wrote: On 05/13/10 06:15 PM, johan...@opensolaris.org wrote: ... I'm willing to find a compromise. I think that by default mirrors should be able to run without a cfg_cache. Would you agree to enabling publisher/0 for mirrors if the a) depot is started with --set-property publisher.prefix= or b) depot is started with --cfg-file? I would also need to change the RespositoryConfig object to allow itself to be created without a pathname. That way we get the repository defaults, but don't try to write the file out to disk. If we're started in mirror mode without --cfg-file, we'd load a RepositoryConfig that way. Does this sound like an acceptable solution to you? I prefer a more generic approach. If the cfg_cache file doesn't exist today, the repositoryconfig sets up some initial defaults. Only required properties don't automatically get one (at the moment, that's only publisher prefix). If the cfg_cache file is present, it should be used. I don't have any desire to cater to poorly administered repositories. I don't like the idea of requiring --cfg-file as that goes against its intended purpose and introduces incosistency in the interface. I think you still don't understand my use case. Say that you've got a depot and a mDNS depot on a machine. The depot is configured in the standard location, and the mDNS depot is configured to mirror the contents of /var/pkg/download. By default the mDNS depot will end up looking for a config file, and find the one that's being used by the standard depot. That's obviously not desireable; I'd like it to be How would it find the configuration file used by the standard depot if it's mirroring /var/pkg/download? The only way that would happen is if you set the repo_dir to the same location as the depot. Why would you do that? possible to stand up one of these mirrors with the minimum amount of fuss. They don't actually need a config file, but could use one if desired. The problem here isn't administrator laziness, so much as it ease of setup and possible problems with sharing the filesystem. I want something that can serve file content out of the download directory, without a lot of bells and whistles. I'm only suggesting that we require cfg-file for mirrors, if the admin wants publisher/0 support and no publisher prefix was specified on the command line. Until I understand the above, this still doesn't make any sense to me. Cheers, -Shawn ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On Thu, May 13, 2010 at 06:53:28PM -0500, Shawn Walker wrote: > On 05/13/10 06:15 PM, johan...@opensolaris.org wrote: > ... > >I'm willing to find a compromise. I think that by default mirrors > >should be able to run without a cfg_cache. Would you agree to enabling > >publisher/0 for mirrors if the a) depot is started with --set-property > >publisher.prefix= or b) depot is started with --cfg-file? > > > >I would also need to change the RespositoryConfig object to allow itself > >to be created without a pathname. That way we get the repository > >defaults, but don't try to write the file out to disk. If we're started > >in mirror mode without --cfg-file, we'd load a RepositoryConfig that > >way. > > > >Does this sound like an acceptable solution to you? > > I prefer a more generic approach. If the cfg_cache file doesn't > exist today, the repositoryconfig sets up some initial defaults. > Only required properties don't automatically get one (at the moment, > that's only publisher prefix). > > If the cfg_cache file is present, it should be used. I don't have > any desire to cater to poorly administered repositories. > > I don't like the idea of requiring --cfg-file as that goes against > its intended purpose and introduces incosistency in the interface. I think you still don't understand my use case. Say that you've got a depot and a mDNS depot on a machine. The depot is configured in the standard location, and the mDNS depot is configured to mirror the contents of /var/pkg/download. By default the mDNS depot will end up looking for a config file, and find the one that's being used by the standard depot. That's obviously not desireable; I'd like it to be possible to stand up one of these mirrors with the minimum amount of fuss. They don't actually need a config file, but could use one if desired. The problem here isn't administrator laziness, so much as it ease of setup and possible problems with sharing the filesystem. I want something that can serve file content out of the download directory, without a lot of bells and whistles. I'm only suggesting that we require cfg-file for mirrors, if the admin wants publisher/0 support and no publisher prefix was specified on the command line. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On 05/13/10 06:15 PM, johan...@opensolaris.org wrote: ... I'm willing to find a compromise. I think that by default mirrors should be able to run without a cfg_cache. Would you agree to enabling publisher/0 for mirrors if the a) depot is started with --set-property publisher.prefix= or b) depot is started with --cfg-file? I would also need to change the RespositoryConfig object to allow itself to be created without a pathname. That way we get the repository defaults, but don't try to write the file out to disk. If we're started in mirror mode without --cfg-file, we'd load a RepositoryConfig that way. Does this sound like an acceptable solution to you? I prefer a more generic approach. If the cfg_cache file doesn't exist today, the repositoryconfig sets up some initial defaults. Only required properties don't automatically get one (at the moment, that's only publisher prefix). If the cfg_cache file is present, it should be used. I don't have any desire to cater to poorly administered repositories. I don't like the idea of requiring --cfg-file as that goes against its intended purpose and introduces incosistency in the interface. Cheers, -Shawn ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On Thu, May 13, 2010 at 05:44:56PM -0500, Shawn Walker wrote: > Again, future functionality may mean we give out bad or meaningless > configuration data. If the configuration data is bad, it is the > responsibility of the administrator to correct it. I don't want to > start making design decisions based on the bad behaviour of > consumers. You seem to be arguing that this one point invalidates all of my other arguments. I'm not so sure, and moreover, I think you're not taking note of the fact that we could come up with ways to make it harder to misconfigure the the depot. I'm not saying that those have to be implemented now, or even for this release, but if we can make some observations about routine mis-configurations, maybe there's a possibility of simplifying the job of the administrator later on? What do you think? > I can't fix garbage in == garbage out. I never said that you should. I was merely suggesting that we think about ways to improve this in the future, to make an out of the box configuration more simple and less error prone. > For example, I'm getting ready to change the depot to add the > Expires/Cache-Control headers to almost every operation, and that > depends on the refresh_seconds property stored in the cfg_cache > file. I would suggest that if you're in mirror mode, you pick a value for this. Mirrors are serving content that doesn't have an expiration date tied to refresh_seconds. There are no manifests or catalogs in a mirror. > To see what's actually happening in the client scenario (where > /var/pkg/download is being served), it'd be interesting to see if > the cfg_cache file in the image's directory has changed after > running the mirror, and if not, a dump of the repository > configuration sometime during repo init. The mdns mirror runs both read-only and mirror, so it shouldn't be modifying any files at all. > I don't see the need for a separate set of options. Note that only > thing I asked for was making the publisher operation available > conditionally based on whether the "publisher" "prefix" property was > set -- that's all. That doesn't require a separate set of options > for what you want to do. I thought you were asking for this if the value was set in cfg_cache? > I can see we're probably not going to come to agreement on this, so > I leave it up to you. But know that I will likely be re-enabling > this again in the near future based on the criteria I already set > above. I'm willing to find a compromise. I think that by default mirrors should be able to run without a cfg_cache. Would you agree to enabling publisher/0 for mirrors if the a) depot is started with --set-property publisher.prefix= or b) depot is started with --cfg-file? I would also need to change the RespositoryConfig object to allow itself to be created without a pathname. That way we get the repository defaults, but don't try to write the file out to disk. If we're started in mirror mode without --cfg-file, we'd load a RepositoryConfig that way. Does this sound like an acceptable solution to you? -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On 05/13/10 05:25 PM, johan...@opensolaris.org wrote: On Thu, May 13, 2010 at 04:50:10PM -0500, Shawn Walker wrote: ... Right, which is why I pointed out the information would be optional. Again, the functionality is *already* available today, and those bad configurations are already available today. So removing functionality based on existing bad configurations doesn't seem like a reasonable justification to me. So we should continue to give out bad or meaningless configuration data? Again, future functionality may mean we give out bad or meaningless configuration data. If the configuration data is bad, it is the responsibility of the administrator to correct it. I don't want to start making design decisions based on the bad behaviour of consumers. I have sent numerous notices about verifying the validity of the cfg_cache file during past heads up messages, including the one where the publisher prefix was required. Administrators have been adequately forewarned. Again, I don't think existing bogus config data is a valid reason to remove this from the list of possible operations. When we decide later to actually use this functionality, then the same bad configurations will still be a problem. I've given more examples than just bogus config data. Regardless, you're correct that when you actually decide to use this functionality the bad configurations will still be a problem. How do you plan to deal with this? I've proposed a solution that results in correct behavior for mirrors, and doesn't harm origins. It's impossible to know if configuration data is valid in some scenarios. For example, if you configure the repository as belonging to 'example-pub' when you first create it, there's no existing package data to say "this probably isn't right". And that's also a perfectly legitimate configuration. I can't fix garbage in == garbage out. The depot BUI currently requires (and assumes) that a valid configuration file is available. If you really need to be able to run without a pre-supplied one, then you need to auto-generate one for the client mirror case as part of this putback. I didn't realise that this functionality would be attempting to use the client's cfg_cache. It's not attempting to use the client's cfg_cache. I'm pointing out a problem with using the client image-root as the image root for this depot. We tested the BUI without configuration file earlier in this code review and found that everything worked. I don't understand this particular objection. Everything works *at the moment*, in part because the repository configuration largely isn't used in mirror mode. But there's an assumption made by the both the pkg.server.api and the BUI templates that the configuration is valid (that is, all required properties are present). For example, I'm getting ready to change the depot to add the Expires/Cache-Control headers to almost every operation, and that depends on the refresh_seconds property stored in the cfg_cache file. To see what's actually happening in the client scenario (where /var/pkg/download is being served), it'd be interesting to see if the cfg_cache file in the image's directory has changed after running the mirror, and if not, a dump of the repository configuration sometime during repo init. ... We do really have a reason. As I said before, it is *planned* (and there are already bugs open for this) to take advantage of this information in the BUI, etc. You're not making any attempt to answer the previous point about depot architecture. Would you rather I introduce a separate set of repository and depot options for mDNS mode? Previously, you were opposed to something this specific. I don't see the need for a separate set of options. Note that only thing I asked for was making the publisher operation available conditionally based on whether the "publisher" "prefix" property was set -- that's all. That doesn't require a separate set of options for what you want to do. I don't see the point of removing functionality that I'm only going to add back later that's going to hit some of the same potential issues that you're indicating above. I'm removing the functionality now, because it's not providing anything useful to the clients in its current form. In order to get this information to be useful to the clients, we need the new on-disk format, and the ability to determine, based upon the files on disk, what publishers the mirror contains. I can't guess what your implementation of this will look like; it's awkward for me to write code to interface with something that doesn't yet exist. But you don't need to do any of that. This is essentially not removing what's already there. It has 0 impact otherwise on what you've already done. I can see we're probably not going to come to agreement on this, so I leave it up to you. But know that I will likely be re-enabling this again in the near future based on th
Re: [pkg-discuss] Code Review: mDNS mirror detection
On Thu, May 13, 2010 at 04:50:10PM -0500, Shawn Walker wrote: > >I think you've misunderstood what I'm suggesting. I'm not saying that > >we should have two configuration files. Rather, I'm saying that if we > >want to avoid arbitrary mirror configuration, we should deprecate the > >existing configuruation file, auto-generate config information for all > >cases where it's possible, and then have a new configuration file that's > >required for origins and optional for mirrors, or whatever permutation > >makes sense. > > That's fine, but that doesn't mean we need to remove the existing > cfg_cache logic to achieve what you want. I think you still don't understand. I said that this was one possible solution to the problem, not the only solution. > Right, which is why I pointed out the information would be optional. > Again, the functionality is *already* available today, and those bad > configurations are already available today. So removing > functionality based on existing bad configurations doesn't seem like > a reasonable justification to me. So we should continue to give out bad or meaningless configuration data? > Again, I don't think existing bogus config data is a valid reason to > remove this from the list of possible operations. When we decide > later to actually use this functionality, then the same bad > configurations will still be a problem. I've given more examples than just bogus config data. Regardless, you're correct that when you actually decide to use this functionality the bad configurations will still be a problem. How do you plan to deal with this? I've proposed a solution that results in correct behavior for mirrors, and doesn't harm origins. > The depot BUI currently requires (and assumes) that a valid > configuration file is available. If you really need to be able to > run without a pre-supplied one, then you need to auto-generate one > for the client mirror case as part of this putback. I didn't > realise that this functionality would be attempting to use the > client's cfg_cache. It's not attempting to use the client's cfg_cache. I'm pointing out a problem with using the client image-root as the image root for this depot. We tested the BUI without configuration file earlier in this code review and found that everything worked. I don't understand this particular objection. > >Also, in terms of overall depot architecture, you said that you're > >prefer the mDNS mirror not to have a separate set of options in the > >repository or depot. This means that in src/depot.py we do a little bit > >of setup: the plugin, and mandatory repository options, but then don't > >treat this as a special case. It's not obvious to me how we handle the > >publisher/0 case unless we make the mdns mirror it's own option. I'd > >prefer to generalize this to all mirrors until we really have a reason > >to provide configuration information through a mirror's publisher/0 > >operation. > > We do really have a reason. As I said before, it is *planned* (and > there are already bugs open for this) to take advantage of this > information in the BUI, etc. You're not making any attempt to answer the previous point about depot architecture. Would you rather I introduce a separate set of repository and depot options for mDNS mode? Previously, you were opposed to something this specific. > I don't see the point of removing functionality that I'm only going > to add back later that's going to hit some of the same potential > issues that you're indicating above. I'm removing the functionality now, because it's not providing anything useful to the clients in its current form. In order to get this information to be useful to the clients, we need the new on-disk format, and the ability to determine, based upon the files on disk, what publishers the mirror contains. I can't guess what your implementation of this will look like; it's awkward for me to write code to interface with something that doesn't yet exist. > Given all of what you said above, I'd argue that there's a large gap > here in implementation, because valid configuration information is > expected and required for the BUI. I don't understand this comment. Would you please clarify? Thanks, -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On 05/13/10 04:31 PM, johan...@opensolaris.org wrote: On Thu, May 13, 2010 at 03:46:02PM -0500, Shawn Walker wrote: An alternate configuration file because someone screwed up the actual configuration file doesn't seem logical :/ That's a level of complexity I don't think I want to go to. I think you've misunderstood what I'm suggesting. I'm not saying that we should have two configuration files. Rather, I'm saying that if we want to avoid arbitrary mirror configuration, we should deprecate the existing configuruation file, auto-generate config information for all cases where it's possible, and then have a new configuration file that's required for origins and optional for mirrors, or whatever permutation makes sense. That's fine, but that doesn't mean we need to remove the existing cfg_cache logic to achieve what you want. Nevermind that even if you have disabled the publisher response, that doesn't change what the depot BUI is going to show (the depot BUI is supposed to show what publisher's data is being mirrored). Perhaps, but it's not possible to know this for the mDNS case, at least until we can force all mDNS mirrors onto the proposed on-disk format. Right, which is why I pointed out the information would be optional. Again, the functionality is *already* available today, and those bad configurations are already available today. So removing functionality based on existing bad configurations doesn't seem like a reasonable justification to me. ... Except you're actually *removing* this feature now, so it's not really adding it. Right now, a mirror will respond to the publisher response based on the configuration provided. This will only happen if the user is foolish enough to ask the client to use the mirror as a source of configuration data. I'm merely pruning this from the set of available operations for this class of service, not removing the feature entirely. I get that you don't want to preclude configuration from a mirror on the premise that someday, somebody may get this right, under circumstances where we can reliably publish information about what's contained in the repository; however, that's not the case today. The single origin mirrors on SWAN have bogus config data, and the mDNS mirror, in its current form, can't provide reasonable config data. Again, I don't think existing bogus config data is a valid reason to remove this from the list of possible operations. When we decide later to actually use this functionality, then the same bad configurations will still be a problem. I'm not trying to be difficult, but at least for now there are a number of challenges with providing this information for mDNS mirrors. In particular, there's no easy way to tell the difference between a client's cfg_cache and a depot's cfg_cache. If I'm looking at an image and open what I think is a depot cfg_cache, but it's really a client one the depot goes *boom*. That's one reason why the mDNS mirror case needs to be able to run without a configuration file, at least until we can disambiguate the two. The depot BUI currently requires (and assumes) that a valid configuration file is available. If you really need to be able to run without a pre-supplied one, then you need to auto-generate one for the client mirror case as part of this putback. I didn't realise that this functionality would be attempting to use the client's cfg_cache. Note that you can already specify an alternate location for a configuration file. For now, I'd detect the difference by looking for a [property] section in the config. If it exists, it's an image configuration--that's been true for a long time now. Also, in terms of overall depot architecture, you said that you're prefer the mDNS mirror not to have a separate set of options in the repository or depot. This means that in src/depot.py we do a little bit of setup: the plugin, and mandatory repository options, but then don't treat this as a special case. It's not obvious to me how we handle the publisher/0 case unless we make the mdns mirror it's own option. I'd prefer to generalize this to all mirrors until we really have a reason to provide configuration information through a mirror's publisher/0 operation. We do really have a reason. As I said before, it is *planned* (and there are already bugs open for this) to take advantage of this information in the BUI, etc. I don't see the point of removing functionality that I'm only going to add back later that's going to hit some of the same potential issues that you're indicating above. Given all of what you said above, I'd argue that there's a large gap here in implementation, because valid configuration information is expected and required for the BUI. I was thinking you could add a set of unit tests that simply return successful if pybonjour couldn't be imported. Since PyBonjour isn't in the gate, and we have no guarantee that the mdns daemon is running, how a
Re: [pkg-discuss] Code Review: mDNS mirror detection
On Thu, May 13, 2010 at 03:46:02PM -0500, Shawn Walker wrote: > An alternate configuration file because someone screwed up the > actual configuration file doesn't seem logical :/ > > That's a level of complexity I don't think I want to go to. I think you've misunderstood what I'm suggesting. I'm not saying that we should have two configuration files. Rather, I'm saying that if we want to avoid arbitrary mirror configuration, we should deprecate the existing configuruation file, auto-generate config information for all cases where it's possible, and then have a new configuration file that's required for origins and optional for mirrors, or whatever permutation makes sense. > Nevermind that even if you have disabled the publisher response, > that doesn't change what the depot BUI is going to show (the depot > BUI is supposed to show what publisher's data is being mirrored). Perhaps, but it's not possible to know this for the mDNS case, at least until we can force all mDNS mirrors onto the proposed on-disk format. > Yes, but it will still be useful at a later date to know *which* publishers. > > And actually, over time, the data is going to become partitioned by > publisher, it won't be in one big blob as it it's stored now in the > case you're describing. Based upon the format you've proposed, I agree that mirrors that use the pkg(5) format will eventually become more partitioned over time. However, this still seems out of scope for this change. When we have support for partitioning on-disk datasets by publisher, it would make a lot of sense to add this support to the depot configuration information, provided that the depot can determine what publishers it's mirroring by looking at the contents of its repository. > Except you're actually *removing* this feature now, so it's not > really adding it. Right now, a mirror will respond to the publisher > response based on the configuration provided. This will only happen if the user is foolish enough to ask the client to use the mirror as a source of configuration data. I'm merely pruning this from the set of available operations for this class of service, not removing the feature entirely. I get that you don't want to preclude configuration from a mirror on the premise that someday, somebody may get this right, under circumstances where we can reliably publish information about what's contained in the repository; however, that's not the case today. The single origin mirrors on SWAN have bogus config data, and the mDNS mirror, in its current form, can't provide reasonable config data. I'm not trying to be difficult, but at least for now there are a number of challenges with providing this information for mDNS mirrors. In particular, there's no easy way to tell the difference between a client's cfg_cache and a depot's cfg_cache. If I'm looking at an image and open what I think is a depot cfg_cache, but it's really a client one the depot goes *boom*. That's one reason why the mDNS mirror case needs to be able to run without a configuration file, at least until we can disambiguate the two. Also, in terms of overall depot architecture, you said that you're prefer the mDNS mirror not to have a separate set of options in the repository or depot. This means that in src/depot.py we do a little bit of setup: the plugin, and mandatory repository options, but then don't treat this as a special case. It's not obvious to me how we handle the publisher/0 case unless we make the mdns mirror it's own option. I'd prefer to generalize this to all mirrors until we really have a reason to provide configuration information through a mirror's publisher/0 operation. > >>I was thinking you could add a set of unit tests that simply return > >>successful if pybonjour couldn't be imported. > > > >Since PyBonjour isn't in the gate, and we have no guarantee that the > >mdns daemon is running, how about I write a test that enables mirror > >detection and verifies that the client can successfully complete an > >install. That should be sufficient to catch some breakage. > > That might work assuming that only the mirror had the necessary file data. Actually, this is just a test to make sure that even if the mirror-discovery option is enabled, but none of the components are present, we can still proceed without crashing. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On 05/13/10 03:36 PM, johan...@opensolaris.org wrote: On Thu, May 13, 2010 at 01:50:14PM -0500, Shawn Walker wrote: I'm trying to allow the repository administrator to *optionally* specify what publisher's data is being mirrored to assist clients in configuration and/or validation in the future. This seems out of scope, since we don't know what the future requirements for configuration validation may be. See below for why I don't agree. Note that I didn't say you change the validate() check, etc. I'm just wanting to change it so that *if* the administrator has supplied the information, then assume it's valid. This is part of my concern. In the current model, administrators are required to supply a cfg_cache. So far as I can tell, they seem to copy / paste it from somewhere else. If this is the typical behavior, it would be sensible to generate something else by default, and only provide information if explicitly supplied by an administrator. We'd need to create an alternate configuration file in order to accomplish this, since we have no way of determining whether the cfg_cache was put there by someone trying to be careful, or by a quick/dirty cut paste. An alternate configuration file because someone screwed up the actual configuration file doesn't seem logical :/ That's a level of complexity I don't think I want to go to. Again, I don't think bad configuration should preclude the ability to provide potentially valuable information. Nevermind that even if you have disabled the publisher response, that doesn't change what the depot BUI is going to show (the depot BUI is supposed to show what publisher's data is being mirrored). ... Yes, but my suggested change above doesn't preclude that. It does empower the administrator in the single publisher case (which I believe is the more common case for mirrors) to provide more useful information to the client. I think that over time, single publisher mirrors are going to become less common. We've developed a variety of ideas about mirroring content, many of which contain data from multiple upstream sources. Yes, but it will still be useful at a later date to know *which* publishers. And actually, over time, the data is going to become partitioned by publisher, it won't be in one big blob as it it's stored now in the case you're describing. Note that the client doesn't yet use this information, but I would like it to be available so that it could be used if provided. I think that we will want the client to use this information. When it actually becomes possible to determine what publishers are being mirrored, I would imagine that we'd want to change the mirror discovery code to only include sources for publishers we're interested in downloading data from. Based upon the architecture we have right now, though, it's premature to add such a feature. Except you're actually *removing* this feature now, so it's not really adding it. Right now, a mirror will respond to the publisher response based on the configuration provided. If that wasn't already true, I might agree with you. I was thinking you could add a set of unit tests that simply return successful if pybonjour couldn't be imported. Since PyBonjour isn't in the gate, and we have no guarantee that the mdns daemon is running, how about I write a test that enables mirror detection and verifies that the client can successfully complete an install. That should be sufficient to catch some breakage. That might work assuming that only the mirror had the necessary file data. Cheers, -Shawn ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On Thu, May 13, 2010 at 01:50:14PM -0500, Shawn Walker wrote: > I'm trying to allow the repository administrator to *optionally* > specify what publisher's data is being mirrored to assist clients in > configuration and/or validation in the future. This seems out of scope, since we don't know what the future requirements for configuration validation may be. > Note that I didn't say you change the validate() check, etc. I'm > just wanting to change it so that *if* the administrator has > supplied the information, then assume it's valid. This is part of my concern. In the current model, administrators are required to supply a cfg_cache. So far as I can tell, they seem to copy / paste it from somewhere else. If this is the typical behavior, it would be sensible to generate something else by default, and only provide information if explicitly supplied by an administrator. We'd need to create an alternate configuration file in order to accomplish this, since we have no way of determining whether the cfg_cache was put there by someone trying to be careful, or by a quick/dirty cut paste. > I'm also not yet convinced that multi-publisher mirrors are the > common case. It seems like that would make change management (what > bits are available) difficult. Perhaps they're not common now, but the mDNS mirrors will have content from a variety of different sources. > I just want to ensure that the ability to use this potentially > valuable information (when correctly configured) isn't unnecessarily > precluded based on poorly configured repositories. Bad > configuration can always be troublesome. I agree in principle, but I think we're arguing about the nuts and bolts. > Yes, but my suggested change above doesn't preclude that. It does > empower the administrator in the single publisher case (which I > believe is the more common case for mirrors) to provide more useful > information to the client. I think that over time, single publisher mirrors are going to become less common. We've developed a variety of ideas about mirroring content, many of which contain data from multiple upstream sources. > Note that the client doesn't yet use this information, but I would > like it to be available so that it could be used if provided. I think that we will want the client to use this information. When it actually becomes possible to determine what publishers are being mirrored, I would imagine that we'd want to change the mirror discovery code to only include sources for publishers we're interested in downloading data from. Based upon the architecture we have right now, though, it's premature to add such a feature. > >>tests/ > >> Possible to add a simple unit test of some kind? > > I was thinking you could add a set of unit tests that simply return > successful if pybonjour couldn't be imported. Since PyBonjour isn't in the gate, and we have no guarantee that the mdns daemon is running, how about I write a test that enables mirror detection and verifies that the client can successfully complete an install. That should be sufficient to catch some breakage. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On 05/13/10 01:50 PM, Shawn Walker wrote: ... I'm also not yet convinced that multi-publisher mirrors are the common case. It seems like that would make change management (what bits are available) difficult. And yes, I'm aware that the client storage directory is being used as a multi-publisher mirror. But remember that the upcoming on-disk format changes mean that the client will start storing this data per-publisher, so long-term, a unified cache with data mixed in from different publishers will not be the common case. Cheers, -Shawn ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On 05/13/10 01:18 PM, johan...@opensolaris.org wrote: On Wed, May 12, 2010 at 10:50:07PM -0500, Shawn Walker wrote: On 05/12/10 09:01 PM, johan...@opensolaris.org wrote: http://muskoka.eng/~johansen/webrev-pkg-mdns-2/ modules/server/depot.py: line 107: Upon further reflection, I'd like to not automatically disable the publisher operation just because mirror mode is active. Specifically, I'd like to see it omitted from the list of operations in __init__ based on whether the publisher prefix property is set for the cfg_cache. I'm not sure I understand what case you're trying to address here. A mirror can serve content for multiple publishers. I'm trying to allow the repository administrator to *optionally* specify what publisher's data is being mirrored to assist clients in configuration and/or validation in the future. Note that I didn't say you change the validate() check, etc. I'm just wanting to change it so that *if* the administrator has supplied the information, then assume it's valid. I'm also not yet convinced that multi-publisher mirrors are the common case. It seems like that would make change management (what bits are available) difficult. ... Allowing the client to use the mirror as a source of configuration information when we may not know all of the different origins of the content, and know the data is frequently wrong seems troublesome. I just want to ensure that the ability to use this potentially valuable information (when correctly configured) isn't unnecessarily precluded based on poorly configured repositories. Bad configuration can always be troublesome. In your on-disk format proposal, you claim that we'll always know the source of the content that's stored on-disk. I'm not sure how this will That assumption is based on the fact that the prefix has been required for some time, and for origins, it definitely has to match the actual content for the client to get useful results. work for existing mirrors, but for new mirrors we could easily auto-generate the publisher prefixes by looking at what repositories we're serving. This seems like a more roubust long-term solution. Yes, but my suggested change above doesn't preclude that. It does empower the administrator in the single publisher case (which I believe is the more common case for mirrors) to provide more useful information to the client. Note that the client doesn't yet use this information, but I would like it to be available so that it could be used if provided. ... tests/ Possible to add a simple unit test of some kind? What did you have in mind? The mDNS stuff is disabled by default, and we don't currently distribute PyBonjour. The test client and server must be running the mdns daemon in order to publish and recieve the advertisement. I've tested this by hand, but I'm not sure what we could test that's easily automated. I'm sure it wouldn't be easy, but it'd be nice to ensure it doesn't accidentally break in the future. I was thinking you could add a set of unit tests that simply return successful if pybonjour couldn't be imported. If it could be, I was hoping you could start the client & server and then possibly check the results of MirrorDetect.locate() directly? Cheers, -Shawn ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On Wed, May 12, 2010 at 10:50:07PM -0500, Shawn Walker wrote: > On 05/12/10 09:01 PM, johan...@opensolaris.org wrote: > > http://muskoka.eng/~johansen/webrev-pkg-mdns-2/ > > modules/server/depot.py: > line 107: Upon further reflection, I'd like to not automatically > disable the publisher operation just because mirror mode is > active. Specifically, I'd like to see it omitted from the > list of operations in __init__ based on whether the publisher > prefix property is set for the cfg_cache. I'm not sure I understand what case you're trying to address here. A mirror can serve content for multiple publishers. Orthogonally, there's another issue with how this data is currently generated and distributed. I took a look at the data from the publisher/0 for mirrors on SWAN. The majority of these mirrors aren't returning any correct configuration information. My sense is that there's a RFE lurking here for better configuration of publisher/0 information if none is specified when the depot is set up. Allowing the client to use the mirror as a source of configuration information when we may not know all of the different origins of the content, and know the data is frequently wrong seems troublesome. In your on-disk format proposal, you claim that we'll always know the source of the content that's stored on-disk. I'm not sure how this will work for existing mirrors, but for new mirrors we could easily auto-generate the publisher prefixes by looking at what repositories we're serving. This seems like a more roubust long-term solution. > modules/server/repositoryconfig.py: > lines 61-62, 71-72, 82-83: nit: just return instead of assigning to s > as is done on lines 91, 98, etc.? Fixed, thanks. > pkgdefs/Makefile: > line 21: copyright needs update This file actually doesn't need to be in the changeset anymore. Let me see about removing this. > tests/ > Possible to add a simple unit test of some kind? What did you have in mind? The mDNS stuff is disabled by default, and we don't currently distribute PyBonjour. The test client and server must be running the mdns daemon in order to publish and recieve the advertisement. I've tested this by hand, but I'm not sure what we could test that's easily automated. Thanks for taking another look at this. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On 05/12/10 09:01 PM, johan...@opensolaris.org wrote: Folks, I have a new webrev for the mDNS changes. At the moment, we have to exclude PyBonjour from the putback. The client works even if PyBonjour isn't present, but you won't be able to make use of the feature. It's possible to download and install PyBonjour yourself, doing something like: $ pfexec easy_install-2.6 pybonjour I wasn't able to reach cr.opensolaris.org from SWAN tonight. I'm not sure if the host is down, or just unroutable at the moment. Until I can post a code review there, one is available on SWAN: http://muskoka.eng/~johansen/webrev-pkg-mdns-2/ modules/server/depot.py: line 107: Upon further reflection, I'd like to not automatically disable the publisher operation just because mirror mode is active. Specifically, I'd like to see it omitted from the list of operations in __init__ based on whether the publisher prefix property is set for the cfg_cache. modules/server/repositoryconfig.py: lines 61-62, 71-72, 82-83: nit: just return instead of assigning to s as is done on lines 91, 98, etc.? pkgdefs/Makefile: line 21: copyright needs update tests/ Possible to add a simple unit test of some kind? Cheers, -Shawn ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
Folks, I have a new webrev for the mDNS changes. At the moment, we have to exclude PyBonjour from the putback. The client works even if PyBonjour isn't present, but you won't be able to make use of the feature. It's possible to download and install PyBonjour yourself, doing something like: $ pfexec easy_install-2.6 pybonjour I wasn't able to reach cr.opensolaris.org from SWAN tonight. I'm not sure if the host is down, or just unroutable at the moment. Until I can post a code review there, one is available on SWAN: http://muskoka.eng/~johansen/webrev-pkg-mdns-2/ This should address most of the concerns from the previous rounds of code review. Let me know if you have any additonal questions or suggestions. Thanks, -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
johan...@opensolaris.org wrote: > > Have you tested static DNS-SD? > > Sadly, I don't have a static dns server at my disposal. Shouldn't be hard to set one up on a zone or vbox instance, and configure another zone or vbox instance to use it. > > - Is there any reason not to turn discovery on by default? > > Principle of least surprise? I wouldn't expect my client to use mirrors > I hadn't configured, and might not want that if I'm running the client > in an insecure environment. Hm. We can always revisit, but I would think given that we're using secure hashes for all the files, it should be perfectly safe to autoconfigure mirrors. I agree that we need more infrastructure in place (manifest signing at the very least) before we can autoconfigure multiple origins, but I would think that autoconfiguration on by default for mirrors would allow a site to get people to use local resources without having to badger people into configuring something. Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On Thu, Apr 29, 2010 at 05:35:18PM -0700, Danek Duvall wrote: > johan...@opensolaris.org wrote: > > > http://cr.opensolaris.org/~johansen/webrev-6957/ > > Looks pretty nice. Thanks. :) > Why did you choose to expose the interface through a new daemon and > service, rather than simply as an option to the existing daemon? Should > the service just be an instance of pkg/server? If you really needed a new > service, couldn't it have just used a slightly enhanced svc-pkg-depot? This was due to a misunderstanding I had with Stephen. I thought he wanted me to create new dameon, he actually wanted me to create a new lib/svc/method. I'll to make these changes to combine the pkg.mdnsd with svc-pkg-mdns sometime this week. > Have you tested static DNS-SD? Sadly, I don't have a static dns server at my disposal. > .hgignore: > > - Might as well just ignore everything in man ending in .1, .1m, or .5. Ok. > depot.py: > > - Is there any reason to expose --file-root if you're not exposing > --llmirror? I assumed that only callers that were enabling mdns mode would need --llmirror. In that case, it seemed to make sense to hide this option a bit, since the approved way of starting this had been through another method. For file-root, it's entirely possible that other deployment scenarios will have file content stored in a place that's not inside of the repo-root, or in a different directory hierarchy. This allows those weird deployments, as well as /var/pkg/download. > pkg.1.txt: > > - Default values for these properties? Will fix. > mdnsd.py: > > - line 45: "sys.argv[0]" instead of "/usr/lib/pkg.mdnsd"? > > - line 82: no need for continuation character Will fix. > imageconfig.py: > > - Is there any reason not to turn discovery on by default? Principle of least surprise? I wouldn't expect my client to use mirrors I hadn't configured, and might not want that if I'm running the client in an insecure environment. > server/depot.py: > > - line 1495: "registriation" -> "registration" Thanks, will fix. > copyright: > > - No need for the first three lines. Ok. > package:pkg: > > - line 6: gcos-field probably should be "pkg(5) server UID" Fixed. Thanks, -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
johan...@opensolaris.org wrote: > http://cr.opensolaris.org/~johansen/webrev-6957/ Looks pretty nice. Why did you choose to expose the interface through a new daemon and service, rather than simply as an option to the existing daemon? Should the service just be an instance of pkg/server? If you really needed a new service, couldn't it have just used a slightly enhanced svc-pkg-depot? Have you tested static DNS-SD? .hgignore: - Might as well just ignore everything in man ending in .1, .1m, or .5. depot.py: - Is there any reason to expose --file-root if you're not exposing --llmirror? pkg.1.txt: - Default values for these properties? mdnsd.py: - line 45: "sys.argv[0]" instead of "/usr/lib/pkg.mdnsd"? - line 82: no need for continuation character imageconfig.py: - Is there any reason not to turn discovery on by default? server/depot.py: - line 1495: "registriation" -> "registration" copyright: - No need for the first three lines. package:pkg: - line 6: gcos-field probably should be "pkg(5) server UID" Danek ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On 04/29/10 12:17 PM, johan...@opensolaris.org wrote: On Thu, Apr 29, 2010 at 12:10:14PM -0500, Shawn Walker wrote: On 04/29/10 11:59 AM, johan...@opensolaris.org wrote: On Thu, Apr 29, 2010 at 11:38:02AM -0500, Shawn Walker wrote: On 04/29/10 11:33 AM, johan...@opensolaris.org wrote: On Thu, Apr 29, 2010 at 11:16:49AM -0500, Shawn Walker wrote: No; the 404 is purposefully raised to actual users to prevent accidental exposure of paths or other information we wouldn't want just anyone to see. I think there's an RFE to add a debug option though to make development a bit more convenient. With that said; you should see something in the error logs (on the console) where you started the depot server. I did see these two log messages. Is this what you would have expected? Template 'index.shtml' is incompatible with current server api: Incompatible API version '6' specified; expected: '8'. Ensure that the correct --content-root has been provided to pkg.depotd. If you're running this out of your development workspace; yes, very possibly. As the message says, be certain you've provided the correct content-root. If you want to use your workspace's content root, then --content-root=$WS/src will work. With content-root set to $WS/src, I got the same 404 error but no error messages in the log. That's very odd. Argh, sorry. My mistake. $WS was undefined in my environment. With an actual path to the workspace, I see a page that says: Information about packages is not available when the server is operating in mirror mode. Okay, "works as expected" then. Cheers, -Shawn ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On Thu, Apr 29, 2010 at 12:10:14PM -0500, Shawn Walker wrote: > On 04/29/10 11:59 AM, johan...@opensolaris.org wrote: > >On Thu, Apr 29, 2010 at 11:38:02AM -0500, Shawn Walker wrote: > >>On 04/29/10 11:33 AM, johan...@opensolaris.org wrote: > >>>On Thu, Apr 29, 2010 at 11:16:49AM -0500, Shawn Walker wrote: > No; the 404 is purposefully raised to actual users to prevent > accidental exposure of paths or other information we wouldn't want > just anyone to see. I think there's an RFE to add a debug option > though to make development a bit more convenient. > > With that said; you should see something in the error logs (on the > console) where you started the depot server. > >>> > >>>I did see these two log messages. Is this what you would have expected? > >>> > >>>Template 'index.shtml' is incompatible with current server api: > >>> Incompatible API version '6' specified; expected: '8'. > >>> > >>> Ensure that the correct --content-root has been provided to pkg.depotd. > >> > >>If you're running this out of your development workspace; yes, very > >>possibly. > >> > >>As the message says, be certain you've provided the correct content-root. > >> > >>If you want to use your workspace's content root, then > >>--content-root=$WS/src will work. > > > >With content-root set to $WS/src, I got the same 404 error but no error > >messages in the log. > > That's very odd. Argh, sorry. My mistake. $WS was undefined in my environment. With an actual path to the workspace, I see a page that says: Information about packages is not available when the server is operating in mirror mode. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On 04/29/10 11:59 AM, johan...@opensolaris.org wrote: On Thu, Apr 29, 2010 at 11:38:02AM -0500, Shawn Walker wrote: On 04/29/10 11:33 AM, johan...@opensolaris.org wrote: On Thu, Apr 29, 2010 at 11:16:49AM -0500, Shawn Walker wrote: No; the 404 is purposefully raised to actual users to prevent accidental exposure of paths or other information we wouldn't want just anyone to see. I think there's an RFE to add a debug option though to make development a bit more convenient. With that said; you should see something in the error logs (on the console) where you started the depot server. I did see these two log messages. Is this what you would have expected? Template 'index.shtml' is incompatible with current server api: Incompatible API version '6' specified; expected: '8'. Ensure that the correct --content-root has been provided to pkg.depotd. If you're running this out of your development workspace; yes, very possibly. As the message says, be certain you've provided the correct content-root. If you want to use your workspace's content root, then --content-root=$WS/src will work. With content-root set to $WS/src, I got the same 404 error but no error messages in the log. That's very odd. Assuming that you've done a "make install" in your workspace, setting --content-root to $WS/src should use the $WS/src/web directory for the templates. Going further, any errors that happen should be raised and put in the error log, and they always have been in my experience. To debug this further, you could try changing modules/server/face.py temporarily and moving the call to __render_template outside of the try/except block. At that point; the errors should show up directly in your browser assuming it's not a *true* 404. Cheers, -Shawn ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On Thu, Apr 29, 2010 at 11:38:02AM -0500, Shawn Walker wrote: > On 04/29/10 11:33 AM, johan...@opensolaris.org wrote: > >On Thu, Apr 29, 2010 at 11:16:49AM -0500, Shawn Walker wrote: > >>No; the 404 is purposefully raised to actual users to prevent > >>accidental exposure of paths or other information we wouldn't want > >>just anyone to see. I think there's an RFE to add a debug option > >>though to make development a bit more convenient. > >> > >>With that said; you should see something in the error logs (on the > >>console) where you started the depot server. > > > >I did see these two log messages. Is this what you would have expected? > > > >Template 'index.shtml' is incompatible with current server api: > > Incompatible API version '6' specified; expected: '8'. > > > > Ensure that the correct --content-root has been provided to pkg.depotd. > > If you're running this out of your development workspace; yes, very > possibly. > > As the message says, be certain you've provided the correct content-root. > > If you want to use your workspace's content root, then > --content-root=$WS/src will work. With content-root set to $WS/src, I got the same 404 error but no error messages in the log. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On 04/29/10 11:33 AM, johan...@opensolaris.org wrote: On Thu, Apr 29, 2010 at 11:16:49AM -0500, Shawn Walker wrote: No; the 404 is purposefully raised to actual users to prevent accidental exposure of paths or other information we wouldn't want just anyone to see. I think there's an RFE to add a debug option though to make development a bit more convenient. With that said; you should see something in the error logs (on the console) where you started the depot server. I did see these two log messages. Is this what you would have expected? Template 'index.shtml' is incompatible with current server api: Incompatible API version '6' specified; expected: '8'. Ensure that the correct --content-root has been provided to pkg.depotd. If you're running this out of your development workspace; yes, very possibly. As the message says, be certain you've provided the correct content-root. If you want to use your workspace's content root, then --content-root=$WS/src will work. Cheers, -Shawn ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On Thu, Apr 29, 2010 at 11:16:49AM -0500, Shawn Walker wrote: > No; the 404 is purposefully raised to actual users to prevent > accidental exposure of paths or other information we wouldn't want > just anyone to see. I think there's an RFE to add a debug option > though to make development a bit more convenient. > > With that said; you should see something in the error logs (on the > console) where you started the depot server. I did see these two log messages. Is this what you would have expected? Template 'index.shtml' is incompatible with current server api: Incompatible API version '6' specified; expected: '8'. Ensure that the correct --content-root has been provided to pkg.depotd. -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On 04/29/10 10:54 AM, johan...@opensolaris.org wrote: Hi Shawn, Thanks for looking at this. On Thu, Apr 29, 2010 at 12:32:22AM -0500, Shawn Walker wrote: On 04/26/10 07:36 PM, johan...@opensolaris.org wrote: Webrev is available here: http://cr.opensolaris.org/~johansen/webrev-6957/ src/depot.py: line 149, 219-221, 303-304, 703: nit; options should be in asciibetical order :) Ok, fixed. In some places these options were not in alphabetical order to begin with, which is why I assumed it was okay to add them wherever I pleased. I've tried to put those places back into complete order. Perhaps we could enforce this consistently to avoid this kind of confusion in the future? I try my best to be certain that these are in a consistent order as it benefits the user, especially in documentation. However, like most things, bitrot occurs ;) ... What happens when you try to access the BUI index page for a repository that has no cfg_cache file or that doesn't provide a publisher prefix and is running in --mirror or --llmirror mode? Right now I get: 404 Not Found The path '/' was not found. Is there a way to disable the BUI for this mode? There's not really anything to display to human users in the mirror case. No; the 404 is purposefully raised to actual users to prevent accidental exposure of paths or other information we wouldn't want just anyone to see. I think there's an RFE to add a debug option though to make development a bit more convenient. With that said; you should see something in the error logs (on the console) where you started the depot server. Cheers, -Shawn ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
Hi Shawn, Thanks for looking at this. On Thu, Apr 29, 2010 at 12:32:22AM -0500, Shawn Walker wrote: > On 04/26/10 07:36 PM, johan...@opensolaris.org wrote: > >Webrev is available here: > > > > http://cr.opensolaris.org/~johansen/webrev-6957/ > > src/depot.py: > line 149, 219-221, 303-304, 703: > nit; options should be in asciibetical order :) Ok, fixed. In some places these options were not in alphabetical order to begin with, which is why I assumed it was okay to add them wherever I pleased. I've tried to put those places back into complete order. Perhaps we could enforce this consistently to avoid this kind of confusion in the future? > src/man/pkg.1.txt: > lines 684-689: > From which image will the content be removed? The current BE, or > target BE, or both during operations such as image-update? > Currently, this only happens in the original BE because the > content cache cleanup is called after be activation in modules/ > client/api.py in __finished_execution. Thanks, clarified. > lines 698-703: You may want to define what UUID stands for the first > time it is used. Ok, fixed. > src/modules/client/imageconfig.py: > lines 47-59: Since you're here, and it's just deletes, it'd be nice to > also address bug 11178 (just delete all references to > PURSUE_LATEST and DISPLAY_COPYRIGHTS). Deleted. > src/modules/client/transport/exception.py: > line 315: nit; insert newline here Fixed. > src/modules/server/repository.py: > lines 947-948: Seems like you could drop these and change line 946 > to simply set self.file_root instead? Did you mean lines 938 and 939 in __set_repo_root? If so, then yes, I'll have this assign to self.file_root instead. However, the conditional is necessary since I'd like it to be possible for file-root to be outside of repo-root. > src/svc/svc-pkg-depot: > lines 49-51, 53-56: nit; options in asciibetical order Fixed. > src/modules/client/transport/mdetect.py: > line 70: Incomplete sentence? Changed. > General Comment: > Any chance of this being able to advertise available nfs mounts too > once the client support for filesystem-based access is putback? The client will detect any repository that's advertised following our DNS-SD conventions. It wouldn't be a good idea to run a depot to advertise a NFS-mounted file repository. It would be easy enough to write a separate advertiser, though. We can also advertise services through static DNS, if we ever feel the need. > What happens when you try to access the BUI index page for a > repository that has no cfg_cache file or that doesn't provide > a publisher prefix and is running in --mirror or --llmirror mode? Right now I get: 404 Not Found The path '/' was not found. Is there a way to disable the BUI for this mode? There's not really anything to display to human users in the mirror case. Thanks -j ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On 04/29/10 12:32 AM, Shawn Walker wrote: ... general: Why was --file-root needed? This seems somewhat tangenital to adding mDNS support and there was nothing enlightening in the bugs. Is this to support /dev and /release sharing a single file area? And you can ignore this point; I realised this morning that you had to do this since /var/pkg doesn't contain a 'file' directory -- only a 'download' directory which you want to use for serving repository file data. Cheers, -Shawn ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Re: [pkg-discuss] Code Review: mDNS mirror detection
On 04/26/10 07:36 PM, johan...@opensolaris.org wrote: Folks, The following webrev creates a read-only mirror mode for the depot that allows it to serve the contents of your /var/pkg/download directory and advertise its availability via mDNS. The client can be configured to detect mirrors advertised by mDNS and use them if they're faster than the depot. This may help in environments where the main repository is heavily loaded, but there are many users on the local subnet that may have copies of the server's content. Webrev is available here: http://cr.opensolaris.org/~johansen/webrev-6957/ src/depot.py: line 149, 219-221, 303-304, 703: nit; options should be in asciibetical order :) general: Why was --file-root needed? This seems somewhat tangenital to adding mDNS support and there was nothing enlightening in the bugs. Is this to support /dev and /release sharing a single file area? src/man/pkg.1.txt: lines 684-689: From which image will the content be removed? The current BE, or target BE, or both during operations such as image-update? Currently, this only happens in the original BE because the content cache cleanup is called after be activation in modules/ client/api.py in __finished_execution. lines 698-703: You may want to define what UUID stands for the first time it is used. src/modules/client/imageconfig.py: lines 47-59: Since you're here, and it's just deletes, it'd be nice to also address bug 11178 (just delete all references to PURSUE_LATEST and DISPLAY_COPYRIGHTS). src/modules/client/transport/exception.py: line 315: nit; insert newline here src/modules/server/repository.py: lines 947-948: Seems like you could drop these and change line 946 to simply set self.file_root instead? src/svc/svc-pkg-depot: lines 49-51, 53-56: nit; options in asciibetical order src/modules/client/transport/mdetect.py: line 70: Incomplete sentence? General Comment: Any chance of this being able to advertise available nfs mounts too once the client support for filesystem-based access is putback? What happens when you try to access the BUI index page for a repository that has no cfg_cache file or that doesn't provide a publisher prefix and is running in --mirror or --llmirror mode? Cheers, -Shawn ___ pkg-discuss mailing list pkg-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
