Skip to content
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

Merged
merged 2 commits into from
Oct 16, 2018

Conversation

ExternalReality
Copy link
Contributor

@ExternalReality ExternalReality commented Oct 16, 2018

Description of change

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.

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 from 2.3 to 2.4:

  1. Boostrap with Juju 2.3
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
  1. Upgrade to Juju 2.4
snap refresh juju --channel 2.4 --classic
juju upgrade-juju -m controller
juju upgrade-juju
  1. Inspect the exec-start.sh to see that the bug has in fact been reproduced
juju ssh 0 'cat /lib/systemd/system/jujud-machine-0/exec-start.sh'
  1. Install this PR version of Juju (so you get the fix) and upgrade
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
  1. Wait a few seconds for the files to be rewritten
juju 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

@ExternalReality
Copy link
Contributor Author

// NixDataDir is location for agent binaries on *nix operating systems.
NixDataDir = "/var/lib/juju"

// NixDataDir is location for Juju logs on *nix operating systems.
Copy link
Contributor Author

@ExternalReality ExternalReality Oct 16, 2018

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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)),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How?

Copy link
Member

@manadart manadart left a 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.

@ExternalReality
Copy link
Contributor Author

!!build!!

@ExternalReality
Copy link
Contributor Author

$$MERGE$$

@@ -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")
Copy link
Contributor

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.

Copy link
Contributor Author

@ExternalReality ExternalReality Oct 16, 2018

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.

Copy link
Member

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.

@jujubot jujubot merged commit 2ad0116 into juju:2.4 Oct 16, 2018
@ExternalReality ExternalReality deleted the fix_logging_path branch October 17, 2018 19:00
jameinel added a commit to jameinel/juju that referenced this pull request Oct 25, 2018
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
@jameinel jameinel mentioned this pull request Oct 25, 2018
jujubot added a commit that referenced this pull request Oct 26, 2018
#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
ExternalReality pushed a commit to ExternalReality/juju that referenced this pull request Oct 29, 2018
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
jujubot added a commit that referenced this pull request Oct 30, 2018
#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.*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants