Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
migrations: implement AdoptResources for azure provider #6868
Conversation
| + for resources.Value != nil { | ||
| + for _, resource := range *resources.Value { | ||
| + | ||
| + providerNamespace, parentPath, err := splitResourceType(*resource.Type) |
babbageclunk
Jan 25, 2017
Member
This is the bit I'm least certain about - from looking at the places constructing Resources in the same file this seems to be what the Type attribute is. It generally looks like "Microsoft.Compute/availabilitySets" - so this would have the provider namespace "Microsoft.Compute" and parent path "availabilitySets".
But I don't really get why it would be needed given that we're also passing the type as another field? (And in the resource as well.) So maybe I'm misunderstanding? Or is the redundancy just an artifact of API autogeneration?
babbageclunk
Jan 25, 2017
Member
I've signed up for an Azure trial to try to work questions like this out, but I haven't managed yet.
axw
Jan 25, 2017
Member
Ah, actually I don't think this bit is right.
Microsoft.Compute is the namespace, availabilitySets is the resource type; but it is not a parent resource type, as availability sets don't have a parent.
If there were other bits in between Microsoft.Compute and availabilitySets, the other bits would constitute the parent path.
babbageclunk
Jan 25, 2017
Member
Ok, I had a feeling it might be something like that. I'll change the splitting to take the first component as the the namespace, the last component as the type and the parent path as anything in between.
I'll do a deploy to azure (should have done that first) and then I can see what comes back in the Type field when I call ListResources.
| + logger.Errorf("error getting namespace and path for type %q: %v", *resource.Type, err) | ||
| + } | ||
| + | ||
| + (*resource.Tags)[tags.JujuController] = &controllerUUID |
| + logger.Errorf("error getting namespace and path for type %q: %v", *resource.Type, err) | ||
| + } | ||
| + | ||
| + (*resource.Tags)[tags.JujuController] = &controllerUUID |
axw
Jan 25, 2017
Member
check that the resource isn't already tagged with this controller UUID? is it possible for this function to be reentrant?
babbageclunk
Jan 25, 2017
Member
Good point - I meant to do that check before updating but forgot.
I'm not sure what you mean about reentrancy - you mean, could this be called multiple times with the same controller uuid? Or different ones? I guess I can see it being called more than once with the same uuid, if there was something wrong with the migration master worker.
Do you mean, if it was called in parallel with two different uuids could we end up in an inconsistent state? If the order of the resources coming back was inconsistent between two different calls I think that would cause a problem. Hmm. Or if one of them was delayed for a long time and then continued after the other one had run.
I'm not sure how to handle that - I can think of a couple of options that still seem racy, unless there's a way to make the update conditional on the value of the tag being what we expect (on the azure end rather than ours).
I don't think the migration machinery would allow this to happen (concurrent calls with different uuids) - the second migration would be prevented until the first was finished. Do you think that's good enough or should we try to handle that at this level?
(Or have I completely misunderstood the question? :)
axw
Jan 27, 2017
Member
What I meant was: if this fails half way through, e.g. due to a network failure, will/can it be called again? If so, we should handle a half-processed resource group.
babbageclunk
Jan 27, 2017
Member
Oh, I see! Yes, it can get restarted after crashing halfway through - the migration master will retry the phase if it errors out, so this would be called again on the half-updated group.
I think it would always handle that right (although your comment here will make it more efficient). We're finding the resources by group, based on the model id which hasn't changed. Some of the resources will have the old controller tag and some will have the new one, and they'll all be updated to the new one (barring more network errors).
|
!!chittychitty!! |
babbageclunk
changed the title from
WIP migrations: implement AdoptResources for azure provider
to
migrations: implement AdoptResources for azure provider
Jan 30, 2017
babbageclunk
added some commits
Jan 25, 2017
|
!!chittychitty!! |
| + return errors.Trace(err) | ||
| + } | ||
| + | ||
| + resources, err := groupClient.ListResources(env.resourceGroup, "", nil) |
axw
Jan 31, 2017
Member
I know it's ugly and gross, but the API calls should be wrapped with env.callAPI to take care of rate limiting.
| + err := env.updateResourceControllerTag(&resourceClient, resource, controllerUUID) | ||
| + if err != nil { | ||
| + logger.Errorf("error updating resource tags for %q: %v", *resource.Name, err) | ||
| + failed = append(failed, *resource.Name) |
| +} | ||
| + | ||
| +func (env *azureEnviron) updateResourceControllerTag(client *resources.Client, stubResource resources.GenericResource, controllerUUID string) error { | ||
| + stubTags := *stubResource.Tags |
| + return nil | ||
| + } | ||
| + | ||
| + namespace, parentPath, subtype, err := splitResourceType(*stubResource.Type) |
| + | ||
| + logger.Debugf("updating %s (%s) juju controller uuid to %s", | ||
| + *resource.Name, *resource.Type, controllerUUID) | ||
| + (*resource.Tags)[tags.JujuController] = &controllerUUID |
axw
Jan 31, 2017
Member
resourceTags := toTags(resource.Tags)
resourceTags[tags.JujuController] = controllerUUID
resource.Tags = to.StringMapPtr(resourceTags)
babbageclunk
Jan 31, 2017
Member
Well, I mean it's still horrible, but better to deal with a good old map[string]string.
| +func splitResourceType(resourceType string) (string, string, string, error) { | ||
| + parts := strings.Split(resourceType, "/") | ||
| + if len(parts) < 2 { | ||
| + return "", "", "", errors.Errorf("expected at least 2 parts in resource type %q", resourceType) |
babbageclunk
Jan 31, 2017
Member
Sometimes (fairly often) the type looks like Namespace/type, eg Microsoft.Compute/virtualMachines - in that case there's no parent path.
axw
Jan 31, 2017
Member
Ah right, I was thinking we were passing in the full resource ID, but of course we're just passing in the type.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
Spurious failure - worker/machiner/machiner_test.go:514: MachinerStateSuite.TestMachineAddresses $$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
babbageclunk commentedJan 25, 2017
•
Edited 1 time
-
babbageclunk
Jan 30, 2017
Description of change
The AdoptResources environ method will be used at the end of a model migration to update the controller tag for all of the resources used by the model, so that if the source controller is subsequently destroyed then these resources won't be cleaned up with it.
QA steps
None yet - this method isn't called from the migration master yet.
Documentation changes
N/A
Bug reference
This is part of the fix for https://bugs.launchpad.net/juju/+bug/1648063