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
Azure: Use standard sku Public IP addresses #9499
Conversation
Updates the use of the Azure sdk to a newer (not the newest version yet). Uses the standard SKU for the public IP address (defaults to this atm).
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.
Looking good, a few small things to tidy up
provider/azure/instance.go
Outdated
return errors.Annotatef(err, "creating security rule for %q", ruleName) | ||
} | ||
// veebers: should we care about the actual result or just expect it to happen |
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.
Is this a rhetorical question?
errChan := make(chan error, 1) | ||
// See: resources.Client.CreateOrUpdateByID. | ||
func (client ResourcesClient) CreateOrUpdateByID(ctx context.Context, resourceID string, parameters resources.GenericResource, apiVersion string) (result resources.CreateOrUpdateByIDFuture, err error) { | ||
if tracing.IsEnabled() { |
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.
ditto
2f90a9a
to
d95fcb9
Compare
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 pretty close, but a question about the need to copy SDK code.....
provider/azure/environ.go
Outdated
Location: to.StringPtr(env.location), | ||
Tags: to.StringMapPtr(tags), | ||
Tags: *sanitisedTags, |
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.
We can just leave the original code as is right?
provider/azure/environ.go
Outdated
@@ -1110,11 +1123,12 @@ func (env *azureEnviron) StopInstances(ctx context.ProviderCallContext, ids ...i | |||
cancelResults := make([]error, len(ids)) | |||
for i, id := range ids { | |||
logger.Debugf("canceling deployment for instance %q", id) | |||
sdkCtx := stdcontext.Background() |
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 needs to be done inside the go func below, just after the defer wg.Done() line.
provider/azure/environ.go
Outdated
@@ -1161,10 +1175,12 @@ func (env *azureEnviron) StopInstances(ctx context.ProviderCallContext, ids ...i | |||
} | |||
// The deployment does not exist, so there's nothing more to do. | |||
logger.Debugf("deleting instance %q", id) | |||
sdkCtx := stdcontext.Background() |
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.
ditto etc
d95fcb9
to
4cfc41c
Compare
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.
Let's land this
provider/azure/storage.go
Outdated
@@ -907,7 +949,7 @@ func getStorageAccountKey( | |||
resourceGroup, accountName string, | |||
) (*armstorage.AccountKey, error) { | |||
logger.Debugf("getting keys for storage account %q", accountName) | |||
listKeysResult, err := client.ListKeys(resourceGroup, accountName) | |||
listKeysResult, err := client.ListKeys(context.Background(), resourceGroup, accountName) |
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.
stdCtx?
provider/azure/credentials.go
Outdated
@@ -223,7 +225,7 @@ func (c environProviderCredentials) azureCLICredential( | |||
} | |||
params.ResourceManagerAuthorizer = autorest.NewBearerAuthorizer(resourceManagerAuthorizer.Token()) | |||
|
|||
applicationId, password, err := c.servicePrincipalCreator.Create(params) | |||
applicationId, password, err := c.servicePrincipalCreator.Create(context.Background(), params) |
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.
stdCtx?
provider/azure/credentials.go
Outdated
@@ -248,7 +250,7 @@ func (c environProviderCredentials) accountCredential( | |||
if err != nil { | |||
return cloud.Credential{}, errors.Annotatef(err, "cannot get access token for %s", acc.ID) | |||
} | |||
applicationId, password, err := c.servicePrincipalCreator.Create(azureauth.ServicePrincipalParams{ | |||
applicationId, password, err := c.servicePrincipalCreator.Create(context.Background(), azureauth.ServicePrincipalParams{ |
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.
ditto
provider/azure/networking.go
Outdated
@@ -246,7 +247,7 @@ func networkSecurityRules( | |||
nsgClient network.SecurityGroupsClient, | |||
resourceGroup string, | |||
) ([]network.SecurityRule, error) { | |||
nsg, err := nsgClient.Get(resourceGroup, internalSecurityGroupName, "") | |||
nsg, err := nsgClient.Get(context.Background(), resourceGroup, internalSecurityGroupName, "") |
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.
ditto
provider/azure/environ_test.go
Outdated
@@ -1582,7 +1582,7 @@ func (s *environSuite) TestDestroyControllerErrors(c *gc.C) { | |||
s.sender = azuretesting.Senders{ | |||
s.makeSender(".*/resourcegroups", result), // GET | |||
makeErrorSender("foo"), // DELETE | |||
makeErrorSender("bar"), // DELETE | |||
makeErrorSender("foo"), // DELETE |
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.
the idea was to have different errors, so needs to remain as "bar"
3e6e69f
to
a9a2742
Compare
|
1 similar comment
|
Description of change
As per the bug below, use the standard sku for any public IP address that is created (this allows the use of the standard load balancer etc.)
This branch also updates the azure SKU used to the newest version.
QA steps
When you deploy any machines to an Azure provider, using the Azure we UI inspect the created public IP addresses in the resources viewer and see the SKU states 'standard' (as opposed to 'basic').
Documentation changes
n/a
Bug reference
https://bugs.launchpad.net/juju/+bug/1799238