CA-209401: add static-vdis detach feature for HA #2731
Conversation
which calls the detach function of static-vdis (implemented separately). Signed-off-by: Zheng Li <zheng.li3@citrix.com>
The format of "config" remains backward compatible. As we are moving towards SMAPI3 anyway (where the config is no longer necessary), this is the least interruptive approach with no synchronization/upgrade efforts needed. Signed-off-by: Zheng Li <zheng.li3@citrix.com>
... as suggested by Jon Ludlam. Signed-off-by: Zheng Li <zheng.li3@citrix.com>
|
For review, can only merge after the corresponding SM changes reach trunk-ring3. |
scripts/init.d-attach-static-vdis
Outdated
| # Needed for interface-reconfigure below | ||
| . /etc/xensource-inventory | ||
|
|
||
| RETVAL=0 |
There was a problem hiding this comment.
How do you ensure that this value is not overwritten once it has been set? I think it would be cleaner to let functions return a value rather than using a global variable. You also might want to use https://www.shellcheck.net/.
…stead of restart In the original definition of the init script, "restart" is defined as just a shortcut for "start", and "start" is itself idempotent. The reattempt_on_boot_attach delibrately does another "(re)start" attempt based on this fact as an insurance resort. But with systemd now in place, the semantics of "start/stop/restart" depends on not only the definition of cmdlet entries themselves but also the service's current status as in systemd's book. So a "restart" (even if it's just defined as a nic for "start") usually ended with a autonomous "stop" action first, given if systemd believes the service has already been running at that point. Interestingly, the previous "stop" cmdlet was a non-op, so even after we moved to systemd, the "restart" semantics happened to remaine the same. But this is not the case any more, as we've now added the "stop" implementation. The autonomous "stop" would now cause troubles, because xhad might have been holding one of the static-vdi at that point as the service started succeeds previously. The extra "stop" action would then hung indefinitely. This also contradicts with our intention, which is just to "ensure" the static vdis attaching was successful , rather than detach and rettach them once again. This patch fixes this by calling "start" instead of "restart". It fits the systemd model as a double checker. If systemd believes the service is already running, this will be a non-op. To do this, we also modified the "start" implementation to return proper error code in case of failures. Nonethess, to control service running/stopping directly from xapi was not a clean model to begin with. But at the stage, this is probably the least interruptive changes we can make to let things continue to work. Signed-off-by: Zheng Li <zheng.li3@citrix.com>
|
|
||
|
|
||
| stop() { | ||
| echo -n $"Attempting to detach all statically-configured VDIs" |
There was a problem hiding this comment.
What is echo $"..." doing different from echo "..."? I'm not familiar with this. It is also used above.
There was a problem hiding this comment.
No idea, just inherited from above, and it seems quite pervasive in init.d scripts.
Did a little bit search around, it seems related to i18n (http://wiki.bash-hackers.org/syntax/quoting, search for i18n on the page).
Anyway, it seems no harm to keep it this way.
|
How was this tested? I'd also like to invite someone with a background in HA to review this. |
|
@lindig There is a xenbuilder branch trunk-ca-209401 containing a dev version of these changes (more or less the same here) also corresponding changes on SM. It should be more convenient to test things out there. I've run all the HA tests from nightly suite and majority HA tests from regression suite to make sure the original testcase no longer fails and no regression happens on other HA tests as side effect. |
| driver = read_whole_file(d + "/driver") | ||
| call_backend_detach(driver, config) | ||
| else: | ||
| volume_plugin = read_whole_file(d + "/volume-plugin") |
There was a problem hiding this comment.
For consistency, I think these should be either called volume_{plugin,key,uri} or plugin, key, uri but not using different prefixes.
There was a problem hiding this comment.
It's to be consistent with those already used in the "attach" method above. We'd better keep it this way, or we can change them both together as code refactoring practice later on.
|
Outside my minor comments, I think this is solid but would like to see a second opinion because I am not familiar with HA. Will ask for it at our stand-up. |
|
This also looks good to me and I'm happy to merge once the changes in the SM layer become available (should be soon). However, we need to think about the impact of this on the imminent change from init to systemd of the toolstack daemons. |
|
|
It should work fine if the start/stop order between attach-static-vdis service and xapi service remains the same after the systemd transform. It's just the relationship between those two is a bit tangled with the existing design, and the way of calling service directly from xapi look a bit weird. Anyway, the tests will tell. |
|
The SM changes are now available, so I'll merge this. |
3 participants
No description provided.