David Bustos wrote: > Quoth Tony Nguyen on Wed, Nov 05, 2008 at 03:44:26PM -0800: > >> David Bustos wrote: >> >>> Quoth Tony Nguyen on Wed, Oct 29, 2008 at 04:13:33PM -0700: >>> >>>> The updated and incremental webrevs are at: >>>> >>>> http://cr.opensolaris.org/~tonyn/firewall29Oct2008/ >>>> >>>> http://cr.opensolaris.org/~tonyn/firewall-inc/ >>>> >>> cmd/cmd-inet/usr.lib/inetd/inetd.c >>> > ... > >>> 409: Does this have to happen in inetd? Can it be done in >>> restarter_set_states()? Otherwise, doesn't every restarter have to >>> be changed to accommodate this, which is something you were trying >>> to avoid? >>> >> While it's possible to do this in restarter_set_states(), I'm a little >> concerned about librestart changing a value explicitly passed in by the >> restarter. >> > > Right. It depends on whether service_request should be an explicit > feature or a transparent feature. I suspect the main criterion for that > should be whether either way would bring inherent problems to the > interface, and secondarily, which is more convenient for developers. It > seems to me that transparency is more convenient, but I guess I'm not > clear on whether it's legitimate to use service_request independently of > restarter_inst_validate_aux_fmri(), or not to use service_request even > though restarter_inst_validate_aux_fmri() succeeds. Do you know? > service_request without a valid auxiliary_fmri isn't too useful but I hesitate to mandate all restarters to use service_request at the moment. The most flexible approach would probably be making service_request optional but legitimate only if restarter_inst_validate_aux_fmri() succeeds. That is allowing restarters to determine whether to use the auxiliary_fmri.
The short answer is: I don't have a definitive answer. Once we have enough data to make service_request mandatory, however, I agree that the code should be placed in librestart. >>> 424: Can this also be done in restarter_set_states()? >>> >> Restarter may have different transitioning logic to determine online >> state of its instances. restarter_set_state() will need to understand >> possible restarter specific state transitions to correctly reset >> auxiliary fmri. >> > > I don't understand. Why don't we always clear it if we exit > maintenance? And why do you not clear it if we enter online from > "IIS_IN_REFRESH_METHOD"? > Clearing it when we exit maintenance is probably sufficient. However, I prefer to keep this function here rather than in librestart until we decide to mandate service_request. I originally wanted to clear only when the service becomes online. Using svccfg (and svcadm refresh) to populate the auxiliary_fmri will cause a transition from IIS_IN_REFRESH_METHOD to IIS_ONLINE which resets auxiliary_fmri. >>> cmd/ipf/svc/ipfd.c >>> > ... > >>> 187,192,199: Shouldn't you give the FMRI of the service which you >>> can't process an event for? >>> >> These errors result in a return of -1 to the repository_event_process() >> which will log an error with the FMRI. >> > > Except that error is logged at LOG_DEBUG. repository_event_wait() does > log an error if repository_event_process() fails, but it doesn't include > the FMRI. > Right. It should be logged at LOG_ERR. >>> 238: It appears that you don't use in_maintenance in this case. Did >>> you consider moving the isrpc check up before smf_get_state()? >>> >> we would lose debug information. If we decide to remove the debug >> information then it'd make sense to move this case to the beginning of >> the function or better yet check for isrpc from the caller. >> > > Indeed. My rationale was eliminating error cases, but that's not > decisive, so do whatever you think is best. > ok. >>> 329: Would proceeding if ipfilter is not online be harmful? >>> >> Not harmful just pointless since the ipf command wouldn't succeed and >> the update script will not run if network/ipfilter isn't online. >> > > I suppose modern systems have a lot of memory, so this isn't a big deal. > Hmm, not sure I follow your thought here. If we don't fork an update process, why would there be memory concern. Or do you suggest that smf_get_state() may fail for lack of memory error and not elegantly handled? >>> 401,405,408: Why is it ok to return false if these functions fail? >>> >> Services are not required to have a "rpc" property. The function can't >> verify an RPC service if the property is not found or if there's an >> error getting value. >> > > What if the property exists but has the wrong type? Wouldn't emitting > an error make that easier to debug? Or is it not worth the effort in > your judgement? > Yes, return false but emit errors appropriately. >>> 662: Why don't you have to do anything if a service was deleted? >>> >> The comment is incorrect. The returned fmri is the deleted pg's fmri not >> the fmri of a deleted service or instance. >> > > Actually the comment is correct. If a service or instance is deleted, > you will receive events, and their FMRIs will be placed in your buffer. > (All notification clients receive all deletion events.) > > >> If a service is deleted via >> 'svccfg delete -f fmri', we'll have service specific leftover IPfilter >> configuration. I see this as a corner case. >> > > That was my point. I presume you think this corner case is so rare that > you don't need to handle it? > It is a corner case and I missed instance and service deleting events so didn't think there was a straightforward way to handle it. I verified that we do get the fmri of deleted instance and service so I'll make the suggested change. > >> To support the above force service deletion, we'll need to derive an >> action from a returned pg, i.e. if the deleted pg is an instance's >> 'general' pg, update the instances' IPfilter configuration. Do you think >> we should handle this? >> > > I think you can ignore deletion events for property groups, and only pay > attention to deletion events for instances. I think the ideal program > would handle the events and clean up applicable rules. I don't know how > common it will be, how bad the consequences of not doing it are, or how > much work it would take. I'll leave those for you to judge, though if > you decide not to do it, then you should probably add a comment and file > a change request after integration. > >>> lib/librestart/common/librestart.c >>> _restarter_commit_states(): Don't these changes modify the behavior of >>> restarter_set_states() in an incompatible fashion? That interface >>> has been contracted out to Sun Cluster. Have you contacted them? >>> Shouldn't this be documented in your ARC case? >>> >> These changes address an existing SMF bug >> >> 6236609 svc.startd resets auxiliary state on svcadm mark maintenance >> >> so I'm not sure if it should be mentioned in this ARC case. Sun Cluster >> is now aware of the changes and will update their code once the fix is >> integrated. >> > > Doesn't section 7d of the contract require ARC approval for the change? > Yes, since Sun Cluster is in another consolidation. Thanks, tony
