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

Merge 2.7 on 2019-11-09. #10994

Closed
wants to merge 17 commits into from

Conversation

anastasiamac
Copy link
Contributor

jameinel and others added 17 commits November 21, 2019 14:46
It had been waiting for delay before it would exit, instead we can
notice that we got the request to shutdown early.

This didn't actually block any main threads, because it was only being
triggered by the hub watcher. Still, it is cleaner, IMO.
When running a charm hook, we would notice if the hook exited nonzero,
but if we failed to Start the hook, that error ended up being
suppressed. This patch just puts the error in the same place as existing
exit error.
…54338

juju#10966

## Please provide the following details to expedite Pull Request review:

### Checklist

 - [No] Checked if it requires a [pylibjuju](https://github.com/juju/python-libjuju) change?
 - [N/A] Added [integration tests](https://github.com/juju/juju/tree/develop/tests) for the PR?
 - [N/A ] Do comments answer the question of why design decisions were made?

## Description of change

Don't suppress errors in Start.

When running a charm hook, we would notice if the hook exited nonzero,
but if we failed to Start the hook, that error ended up being
suppressed. This patch just puts the error in the same place as existing
exit error.

## QA steps

You can write a charm that has an invalid 'install' hook. Something like:
```sh
$ cd acceptancetests/repository/charms
$ echo "exit 2" > dummy-sink/hooks/install
$ juju deploy ./dummy-sink
```

Without this patch, it ignores the install hook failure, with this patch you get something like:
```
$ juju status
...
Unit Workload Agent Machine Public address Ports Message
dummy-sink/2* error idle 2 10.210.24.221 hook failed: "install"
```

## Documentation changes

None.

## Bug reference

https://bugs.launchpad.net/juju/+bug/1854338
juju#10933

## Description of change

This is a small cleanup for the http dualPort code. If the http server was shutting down, the old code wouldn't notice until timeout was reached. This didn't actually block anything (as far as I can tell) because the dualListener was opened from a Hub message, which didn't block the other worker from shutting down. So all this really does is cleanup a goroutine a bit earlier.

Still, since I noticed it, I figured it was worth landing. The test doesn't actually fail if you don't exit early, because we can't wait for the openAPIPort to return, because it is triggered from the Hub subscription. Though I think the setup of the test is still relevant (you shouldn't fail if requested to shut down before the timeout occurs.)

## QA steps

New Unit test. You can see how it is triggered if you do 'go test -check.vv' where you can see that it notices right away that shutdown was requested.

## Documentation changes

None.

## Bug reference

None.
juju#10992

With some of the testing infrastructure coming under increasing load, it changes some of the timing for the tests. This is leading to some intermittent test failures where we didn't see them before.

When tests that are full stack tests, like JujuConnSuite, and they create entities, we need to ensure that the internal events have been processed before we continue after creation, or we get spurious test failures like:
```
09:13:48 unitupgrader_test.go:132:
09:13:48 // Initial event
09:13:48 wc.AssertOneChange()
09:13:48 /home/ubuntu/go/src/github.com/juju/juju/core/watcher/watchertest/notify.go:95:
09:13:48 c.Fatalf("watcher sent unexpected change: (_, %v)", ok)
09:13:48 ... Error: watcher sent unexpected change: (_, true)
```
…limiting

juju#10987

Add configurable rate limiting for agent connections.

This work took a sideways step as I needed to write tests for the rate limiting, but all the login tests were using JujuConnSuite. So I changed the login tests to instead be based on statetetesting.StateSuite, which is much lighter.

## QA steps

```sh
juju bootstrap lxd test
juju controller-config agent-ratelimit-max=5
juju controller-config agent-ratelimit-rate=500ms
juju deploy ~jameinel/ubuntu-lite -n 5
for i in {1..5}; do juju add-unit ubuntu-lite -n 5 --to 0,1,2,3,4; done
watch -c -n 1 juju status --color
# in a different shell 
juju ssh -m controller 0
sudo service jujud-machine-0 restart
# observe the reduced rate of reconnection in the status 
```

## Documentation changes

Will need to document the two new controller config values.
juju#10968

## Please provide the following details to expedite Pull Request review:

### Checklist

 - [x] Checked if it requires a [pylibjuju](https://github.com/juju/python-libjuju) change?
_No API server change, only api CLI client._
 - [ ]~~ Added [integration tests](https://github.com/juju/juju/tree/develop/tests) for the PR?~~
 - [ ] ~~Added or updated [doc.go](https://discourse.jujucharms.com/t/readme-in-packages/451) related to packages changed?~~
 - [x] Do comments answer the question of why design decisions were made?

----

## Description of change

Cloud facade has supported the ability to force update and force remove credentials. This PR catches up corresponding CLI.

As a drive-by, I have renamed an internal variable in credential validity to maintain clarity.
 
## QA steps

1. bootstrap with credential A
2. add model with different credential B
3. remove credential B [will fail because a model uses this credential]
4. force remove credential [success]

## Bug reference

Part 2 for https://bugs.launchpad.net/juju/+bug/1852412
# Conflicts:
#	apiserver/admin_test.go
@anastasiamac
Copy link
Contributor Author

$$merge$$

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants