-
Notifications
You must be signed in to change notification settings - Fork 507
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix incorrect machine agent logging path. #9323
Conversation
0a39bb9
to
b6c16dd
Compare
// NixDataDir is location for agent binaries on *nix operating systems. | ||
NixDataDir = "/var/lib/juju" | ||
|
||
// NixDataDir is location for Juju logs on *nix operating systems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code and comment were made to match a subsequent commit to make things easier. This is simply a standard *nix
log dir. You must append "/juju/" to this to get the location for Juju logs on *nix operating systems. Some areas of the code are already doing just that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it confusing that you have the NixDataDir have the juju suffix but not the NixLogDir (which has a copy / paste error in the comment BTW.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, that is very confusing. I coded this in such a way that it will require minimal change to forward port (for whatever that is worth). The copy Pasta was intentional, when this gets forward ported I will change it will change this and a few other things that are incorrect with respect to the changes in this PR.
info := NewAgentInfo( | ||
kind, | ||
name, | ||
dataDir, | ||
paths.MustSucceed(paths.LogDir(series)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great.
I would be inclined to back-port all of #9139 underneath this, as no functionality changed, but it was a significant simplification of the logic under the system package.
!!build!! |
|
@@ -202,12 +202,12 @@ func (s *systemdServiceManager) CreateAgentConf(agentName string, dataDir string | |||
return common.Conf{}, errors.NewNotValid(nil, fmt.Sprintf("agent %q is neither a machine nor a unit", agentName)) | |||
} | |||
|
|||
srvPath := path.Join(paths.NixLogDir, "juju") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work for windows as the NixLogDir isn't right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Buggers, good catch! Wait a minute. AFAIK, this code is Systemd
specific. No Windows
involved here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as I mentioned in #9139, this is systemd-specific and should reside under the systemd package. However, that relocation has not yet been undertaken on account of cyclic dependencies that need resolution.
77a4ecb
to
0f5719a
Compare
Including: * Merge pull request juju#9359 from juju/2.3 * Merge pull request juju#9355 from howbazaar/2.4-engine-backoff * Merge pull request juju#9345 from anastasiamac/only-clear-current-when-asked-lp1796148 * Merge pull request juju#9335 from babbageclunk/mux-worker * Merge pull request juju#9333 from wallyworld/merge-2.3-20181018 * Merge pull request juju#9323 from ExternalReality/fix_logging_path * Merge pull request juju#9327 from mitechie/enable-export-bundle * Merge pull request juju#9315 from manadart/2.4-acceptance-maas-network-bind * Merge pull request juju#9300 from manadart/2.4-provisioner-testing-with-mocks * Merge pull request juju#9287 from manadart/2.4 * Merge pull request juju#9297 from anastasiamac/24-better-debug-lp1779677 * Merge pull request juju#9298 from wallyworld/cmr-controller-connect-robust-2.4 * Merge pull request juju#9251 from howbazaar/bundle-export-series * Merge pull request juju#9293 from babbageclunk/bundle-deploy-panic-2.4 * Merge pull request juju#9285 from howbazaar/2.4-fix-multiwatcher * Merge pull request juju#9264 from mitechie/forward-9235 * Merge pull request juju#9263 from wallyworld/goal-state-units * Merge pull request juju#9255 from howbazaar/2.4-bionic-tests * Merge pull request juju#9203 from wallyworld/go-1.11-2.4 * Merge pull request juju#9191 from howbazaar/2.4-status-retry * Merge pull request juju#9178 from hmlanigan/fixes * Merge pull request juju#9177 from SimonRichardson/ec2-instance-types-2.4 * Merge pull request juju#9181 from babbageclunk/2.3-merge-into-2.4 * Merge pull request juju#9169 from juju/2.3 * Merge pull request juju#9167 from babbageclunk/caasfirewaller-test-fix-2.4 * Merge pull request juju#9166 from babbageclunk/s390x-test-fix-2.4 * Merge pull request juju#9152 from jameinel/2.3-into-2.4 * Merge pull request juju#9080 from vinu2003/juju_exportBundle_timeDelayFix * Merge pull request juju#9138 from wallyworld/charm.v6-dep-update * Merge pull request juju#9137 from anastasiamac/test-order-fix-24 * Merge pull request juju#9134 from anastasiamac/merge-23-into-24-20180829 * Merge pull request juju#9132 from wallyworld/improve-charm-versionstring * Merge pull request juju#9126 from anastasiamac/empty-default-settings-lp1789415-24 * Merge pull request juju#9117 from anastasiamac/init-cmd-lp1785205-24 * Merge pull request juju#9104 from wallyworld/merge-2.3-20180823 Bugs * https://bugs.launchpad.net/juju/+bug/1798485 * https://bugs.launchpad.net/juju/+bug/1793245 * https://bugs.launchpad.net/juju/+bug/1796148 * https://bugs.launchpad.net/juju/+bug/1798001 * https://bugs.launchpad.net/juju/+bug/1793284 * https://bugs.launchpad.net/juju/+bug/1796106 * https://bugs.launchpad.net/juju/+bug/1779677 * https://bugs.launchpad.net/juju/+bug/1795499 * https://bugs.launchpad.net/juju/+bug/1773357 * https://bugs.launchpad.net/juju/+bug/1792299 * https://bugs.launchpad.net/juju/+bug/1782803 * https://bugs.launchpad.net/juju/+bug/1782367 * https://bugs.launchpad.net/juju/+bug/1790626 * https://bugs.launchpad.net/juju/+bug/1789211 * https://bugs.launchpad.net/juju/+bug/1789447 * https://bugs.launchpad.net/juju/+bug/1789415 * https://bugs.launchpad.net/juju/+bug/1785205 * https://bugs.launchpad.net/juju/2.3/+bug/1788554
#9372 ## Description of change Merge of the current tip of 2.4 into 2.5 bringing in patches: prdesc Merge pull request #9359 from juju/2.3 prdesc Merge pull request #9355 from howbazaar/2.4-engine-backoff prdesc Merge pull request #9345 from anastasiamac/only-clear-current-when-asked-lp1796148 prdesc Merge pull request #9335 from babbageclunk/mux-worker prdesc Merge pull request #9333 from wallyworld/merge-2.3-20181018 prdesc Merge pull request #9323 from ExternalReality/fix_logging_path prdesc Merge pull request #9327 from mitechie/enable-export-bundle prdesc Merge pull request #9315 from manadart/2.4-acceptance-maas-network-bind prdesc Merge pull request #9300 from manadart/2.4-provisioner-testing-with-mocks prdesc Merge pull request #9287 from manadart/2.4 prdesc Merge pull request #9297 from anastasiamac/24-better-debug-lp1779677 prdesc Merge pull request #9298 from wallyworld/cmr-controller-connect-robust-2.4 prdesc Merge pull request #9251 from howbazaar/bundle-export-series prdesc Merge pull request #9293 from babbageclunk/bundle-deploy-panic-2.4 prdesc Merge pull request #9285 from howbazaar/2.4-fix-multiwatcher prdesc Merge pull request #9264 from mitechie/forward-9235 prdesc Merge pull request #9263 from wallyworld/goal-state-units prdesc Merge pull request #9255 from howbazaar/2.4-bionic-tests prdesc Merge pull request #9203 from wallyworld/go-1.11-2.4 prdesc Merge pull request #9191 from howbazaar/2.4-status-retry prdesc Merge pull request #9178 from hmlanigan/fixes prdesc Merge pull request #9177 from SimonRichardson/ec2-instance-types-2.4 prdesc Merge pull request #9181 from babbageclunk/2.3-merge-into-2.4 prdesc Merge pull request #9169 from juju/2.3 prdesc Merge pull request #9167 from babbageclunk/caasfirewaller-test-fix-2.4 prdesc Merge pull request #9166 from babbageclunk/s390x-test-fix-2.4 prdesc Merge pull request #9152 from jameinel/2.3-into-2.4 prdesc Merge pull request #9080 from vinu2003/juju_exportBundle_timeDelayFix prdesc Merge pull request #9138 from wallyworld/charm.v6-dep-update prdesc Merge pull request #9137 from anastasiamac/test-order-fix-24 prdesc Merge pull request #9134 from anastasiamac/merge-23-into-24-20180829 prdesc Merge pull request #9132 from wallyworld/improve-charm-versionstring prdesc Merge pull request #9126 from anastasiamac/empty-default-settings-lp1789415-24 prdesc Merge pull request #9117 from anastasiamac/init-cmd-lp1785205-24 prdesc Merge pull request #9104 from wallyworld/merge-2.3-20180823 ## QA steps See individual patches. ## Documentation changes See individual patches. ## Bug reference prdesc https://bugs.launchpad.net/juju/+bug/1798485 prdesc https://bugs.launchpad.net/juju/+bug/1793245 prdesc https://bugs.launchpad.net/juju/+bug/1796148 prdesc https://bugs.launchpad.net/juju/+bug/1798001 prdesc https://bugs.launchpad.net/juju/+bug/1793284 prdesc https://bugs.launchpad.net/juju/+bug/1796106 prdesc https://bugs.launchpad.net/juju/+bug/1779677 prdesc https://bugs.launchpad.net/juju/+bug/1795499 prdesc https://bugs.launchpad.net/juju/+bug/1773357 prdesc https://bugs.launchpad.net/juju/+bug/1792299 prdesc https://bugs.launchpad.net/juju/+bug/1782803 prdesc https://bugs.launchpad.net/juju/+bug/1782367 prdesc https://bugs.launchpad.net/juju/+bug/1790626 prdesc https://bugs.launchpad.net/juju/+bug/1789211 prdesc https://bugs.launchpad.net/juju/+bug/1789447 prdesc https://bugs.launchpad.net/juju/+bug/1789415 prdesc https://bugs.launchpad.net/juju/+bug/1785205 prdesc https://bugs.launchpad.net/juju/2.3/+bug/1788554
juju#9323 `Juju` moves the library directory of juju `systemd` init scripts to a the standard location in a `2.4` upgrade step. This upgrade step, however, was writing the services start script with an incorrect logging path. This PR fixes the issue by introducing a `2.4.5` upgrade step which rewrites the files with the correct log path. This issue only happens on upgrade in `2.4`, due to the error being in an upgrade step, as such we must reproduce the case by upgrading juju from `2.3` to `2.4`: 1. Boostrap with `Juju 2.3` ```bash snap install juju --channel 2.3 --classic # Please ensure you are using the right version juju bootstrap <cloud> test juju deploy ubuntu # Not strictly necessary since we could check the controller machine ``` 2. Upgrade to `Juju 2.4` ```bash snap refresh juju --channel 2.4 --classic juju upgrade-juju -m controller juju upgrade-juju ``` 3. Inspect the `exec-start.sh` to see that the bug has in fact been reproduced ```bash juju ssh 0 'cat /lib/systemd/system/jujud-machine-0/exec-start.sh' ``` 4. Install this PR version of `Juju` (so you get the fix) and upgrade ```bash snap remove juju cd <path to juju repo> go install -v ./... juju upgrade-juju -m controller --build agent # assuming, of course, you have this PR branch checkout out juju upgrade-juju ``` 6. Wait a few seconds for the files to be rewritten ```bash juju ssh 0 'cat /lib/systemd/system/jujud-machine-0/exec-start.sh' ``` The script should be corrected and the services should still work. https://bugs.launchpad.net/juju/+bug/1793284
#9384 #9323 `Juju` moves the library directory of juju `systemd` init scripts to a the standard location in a `2.4` upgrade step. This upgrade step, however, was writing the services start script with an incorrect logging path. This PR fixes the issue by introducing a `2.4.5` upgrade step which rewrites the files with the correct log path. This issue only happens on upgrade in `2.4`, due to the error being in an upgrade step, as such we must reproduce the case by upgrading juju from `2.3` to `2.4`: 1. Boostrap with `Juju 2.3` ```bash snap install juju --channel 2.3 --classic # Please ensure you are using the right version juju bootstrap <cloud> test juju deploy ubuntu # Not strictly necessary since we could check the controller machine ``` 2. Upgrade to `Juju 2.4` ```bash snap refresh juju --channel 2.4 --classic juju upgrade-juju -m controller juju upgrade-juju ``` 3. Inspect the `exec-start.sh` to see that the bug has in fact been reproduced ```bash juju ssh 0 'cat /lib/systemd/system/jujud-machine-0/exec-start.sh' ``` 4. Install this PR version of `Juju` (so you get the fix) and upgrade ```bash snap remove juju cd <path to juju repo> go install -v ./... juju upgrade-juju -m controller --build agent # assuming, of course, you have this PR branch checkout out juju upgrade-juju ``` 6. Wait a few seconds for the files to be rewritten ```bash juju ssh 0 'cat /lib/systemd/system/jujud-machine-0/exec-start.sh' ``` The script should be corrected and the services should still work. https://bugs.launchpad.net/juju/+bug/1793284 ## Please provide the following details to expedite Pull Request review: ---- ## Description of change *Please replace with a description about why this change is needed?* ## QA steps *Please replace with how we can verify that the change works?* ## Documentation changes *Please replace with any notes about how it affects current user workflow? CLI? API?* ## Bug reference *Please add a link to any bugs that this change is related to.*
Description of change
Juju
moves the library directory of jujusystemd
init scripts to a the standard location in a2.4
upgrade step. This upgrade step, however, was writing the services start script with an incorrect logging path. This PR fixes the issue by introducing a2.4.5
upgrade step which rewrites the files with the correct log path.QA steps
This issue only happens on upgrade in
2.4
, due to the error being in an upgrade step, as such we must reproduce the case by upgrading juju from2.3
to2.4
:Juju 2.3
Juju 2.4
exec-start.sh
to see that the bug has in fact been reproducedjuju ssh 0 'cat /lib/systemd/system/jujud-machine-0/exec-start.sh'
Juju
(so you get the fix) and upgradejuju ssh 0 'cat /lib/systemd/system/jujud-machine-0/exec-start.sh'
The script should be corrected and the services should still work.
Bug reference
https://bugs.launchpad.net/juju/+bug/1793284