Fix azure instance queries to account for old completed deployments #13172
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wallyworld
commented
Jul 16, 2021
| @@ -1464,49 +1462,9 @@ func (env *azureEnviron) deleteVirtualMachine( | |||
| return nil | |||
| } | |||
|
|
|||
| // Instances is specified in the Environ interface. | |||
There was a problem hiding this comment.
This block is moved below to be near the other instance query apis
wallyworld
force-pushed
the
azure-instance-lookup
branch
from
acff59a
to
1cf7406
Compare
ycliuhw
reviewed
Jul 16, 2021
| func (env *azureEnviron) allProvisionedInstances( | ||
| ctx context.ProviderCallContext, | ||
| resourceGroup string, | ||
| controllerUUID string, |
There was a problem hiding this comment.
if "controllerUUID" != "", we only look for controller instances rather than instances managed by the current controller?
There was a problem hiding this comment.
yes, we look for controller instances with that controller uuid
| // whose corresponding insts slot is nil. | ||
| // This function returns environs.ErrPartialInstances if the | ||
| // insts slice has not been completely filled. | ||
| func (env *azureEnviron) gatherInstances( |
There was a problem hiding this comment.
Azure doesn't have an API for querying instances by a slice of IDs?
There was a problem hiding this comment.
sadly no - you can ask for instances in a given resource group, but the api has no filter parameter
ycliuhw
approved these changes
Jul 16, 2021
|
|
Merged
jujubot
added a commit
that referenced
this pull request
Jul 16, 2021
#13173 Merge 2.9 #13172 Fix azure instance queries to account for old completed deployments #13166 Add support for bespoke service principals in Azure credentials #13169 SpaceAddress conversion from single candidates instead of a slice #13162 enhance test-network-health #13168 Fix/lp 1936253 #13164 Fixes unit logging appearing on machine log #13160 State logs: index label fields #13163 CLI Deploy: Fix panic The conflicts were mainly due to removing legacy storage from Azure in develop and changing an api version. ``` Conflicts: worker/uniter/runner/jujuc/mocks/context_mock.go version/version.go tests/suites/network/network_health.sh snap/snapcraft.yaml scripts/win-installer/setup.iss provider/azure/environ_test.go provider/azure/environ.go provider/azure/config_test.go ``` ## QA steps See PRs
jujubot
added a commit
that referenced
this pull request
Jul 22, 2021
#13187 Merge from 2.9 to bring forward: - #13184 from manadart/2.9-net-info-iaas-addr-results - #13176 from SimonRichardson/juju-inspect - #13185 from SimonRichardson/logging-unmarshalling-fix - #13181 from SimonRichardson/debug-log-space-labels - #13182 from achilleasa/2.9-fix-error-in-equinix-credential-detection - #13179 from SimonRichardson/update-charm-error-messages - #13178 from manadart/2.9-k9s-no-bootstrap-series - #13175 from manadart/2.9-net-info-ingress-sort - #13174 from SimonRichardson/export-bundle-local-charms - #13170 from manadart/2.9-address-sorting - #13171 from SimonRichardson/deploy-v1-kubernetes - #13172 from wallyworld/azure-instance-lookup - #13167 from jujubot/increment-to-2.9.10 - #13169 from manadart/2.9-space-address-conversion - #13162 from hmlanigan/fix-test-network - #13166 from wallyworld/azure-bespoke-serviceprincipal - #13168 from ycliuhw/fix/lp-1936253 - #13164 from tlm/lp1933548-unit-logging - #13163 from SimonRichardson/deploy-panic-fix - #13160 from SimonRichardson/index-label-logs Conflicts resolved in: - apiserver/facades/agent/uniter/networkinfoiaas.go - core/charm/format.go - core/charm/format_test.go - go.mod - go.sum - provider/azure/config_test.go - provider/azure/environ.go - provider/azure/environ_test.go - provider/azure/environprovider.go - scripts/win-installer/setup.iss - snap/snapcraft.yaml - tests/suites/network/network_health.sh - version/version.go - worker/uniter/runner/jujuc/mocks/context_mock.go
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Using a custom Azure resource group, you can bootstrap and then destroy the controller and re-use the group for the next bootstrap. However, Juju had a buggy method of counting machines in a resource group - it would query the completed Deployment records. Because these records are retained on the group, the next bootstrap would think there are too many machines and exit.
This PR changes how machines are queried - it still has to look for deployments because there's a window where the deployment is created but the machine has not shown up yet. So Juju will now query any incomplete deployments, and augment that list with any machines as well. Any machines found take precedence over deployments. The method to gather instances uses the same logic as for EC2 - a short retry loop to allow the requested instances time to get created.
Also fix some lint issues and the resources API version needed to be bumped.
QA steps
bootstrap to a previously created resource group, add a machine to get 2 azure deployments recorded, destroy the controller, manually delete the resource group contents, and bootstrap again
use the azure portal to see there's 2 Deployments recorded; manually delete resource group content
bootstrap again
juju bootstrap --config resource-group-name=test --no-default-model azure/eastusPreviously this would fail as per the bug.
Bug reference
https://bugs.launchpad.net/juju/+bug/1935979