-
Notifications
You must be signed in to change notification settings - Fork 84
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
VPC: Add Custom Image reconciliation #1942
VPC: Add Custom Image reconciliation #1942
Conversation
Hi @cjschaef. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
cloud/scope/vpc_cluster.go
Outdated
@@ -23,6 +23,7 @@ import ( | |||
|
|||
"github.com/go-logr/logr" | |||
|
|||
"github.com/IBM-Cloud/bluemix-go/crn" |
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.
Can we please not use bluemix just for parsing crn? Since we are not using the main functionality(connecting to ibmcloud), it may create dependency issues in future for capi.
I can see they have just split the crn and directly access the id by index. We can add this into some util func and use it locally.
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.
+1, we purposefully migrated from bluemix-go client to other individual clients from IBM org, wondering if there is any other alternative for the crn validation?!
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 there an alternative golang module I can use instead for parsing CRN's?
Is this a valid alternative?
https://github.com/IBM-Cloud/ibm-cloud-cli-sdk/tree/master/bluemix/crn
It would be helpful to keep the CRN for the image, for future enhancements regarding account details, etc. from a Custom Image CRN that gets supplied.
Should I reimplement the CRN logic within CAPI then?
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.
Should I reimplement the CRN logic within CAPI then?
lets do it..
cloud/scope/vpc_cluster.go
Outdated
if err = s.TagResource(s.IBMVPCCluster.Name, *vpcDetails.CRN); err != nil { | ||
|
||
// NOTE: This tagging is only attempted once. We may wish to refactor in case this single attempt fails. | ||
if err = s.TagResource(s.Name(), *vpcDetails.CRN); err != nil { |
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.
Do we need to make this change as part of this PR?
It seems not relevant to me. Please see whether we can create a separate issue and handle it separately.
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.
I can create a second PR, if that PR can be reviewed and merged prior to this PR, to fix this invalid reference.
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.
I have the separate PR opened for the necessary fixes.
#1949
if err != nil { | ||
return false, fmt.Errorf("error retrieving vpc custom image by id: %w", err) | ||
} | ||
if image == nil { |
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.
Since you are passing id won't it return an error if the image is not found?
Please see whether we need this nil check?
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.
If the image is not found, I thought we got a 404 DetailedResponse back. I am defensively not expecting a non-error (error == nil
) to result in image
always being set.
cloud/scope/vpc_cluster.go
Outdated
|
||
// createCustomImage will create a new VPC Custom Image. | ||
func (s *VPCClusterScope) createCustomImage() (*vpcv1.Image, error) { | ||
if s.IBMVPCCluster.Spec.Image == nil { |
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 nil check already happening in main Reconcile func, please remove here.
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.
Sure I can remove.
if err != nil { | ||
return nil, fmt.Errorf("error unknown failure creating vpc custom image: %w", err) | ||
} | ||
if imageDetails == nil || imageDetails.ID == nil || imageDetails.Name == nil || imageDetails.CRN == nil { |
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.
Why do we need to check all the params separately?
If the error is nil, would imageDetails still be not nil?
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 is done defensively, to make sure the required Image fields are populated from the API response.
} | ||
|
||
// NOTE: This tagging is only attempted once. We may wish to refactor in case this single attempt fails. | ||
if err := s.TagResource(s.Name(), *imageDetails.CRN); err != nil { |
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.
What happens when it fails to tag?
It seems it would again come to createCustomImage func and it will try to create the image again.
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.
If tagging fails, after creating the Custom Image (or any resource with this similar logic, like VPC), we return an error. Next loop through could then find the resource that was created the first attempt, and update the resource Status (in the VPC example, a VPC is likely "ready" in the second attempt), with the expectation it is found by the name used during creation. This will monitor and/or update the resource Status accordingly. As we expect to not try to recreate a resource that was already created, we don't expect to try to retag the resource.
Logic could be added to check tags and apply if missing, but that logic will be more cumbersome and time consuming that desired currently, and I only plan to worry about tags during Delete actions, which are a secondary concern currently.
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.
I can see you have handled this in ReconcileVPC, but here, you don't have a func like GetVPCID. You are checking either from status or from spec crn. If it's there you are validating the image and set the status based on that. Please take care of this here.
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.
Not sure I am following, but if the recommendation here is to update status (SetResourceStatus) prior to tagging, I can move that for Custom Images.
I can handle VPC in a similar fashion then, in a separate PR I expect.
cloud/scope/vpc_cluster.go
Outdated
|
||
// We must have an OperatingSystem value supplied in order to create the Custom Image. | ||
// NOTE(cjschaef): Perhaps we could try defaulting this value, so it isn't required for Custom Image creation. | ||
if s.IBMVPCCluster.Spec.Image.OperatingSystem == nil { |
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.
If you are planning to keep this, you can add this check into webhook.
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.
Sorry, I'm not sure what the webhook expected change would be, can you provide more details?
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 additional API changes, such as enhancing this function?
func (r *IBMVPCCluster) validateIBMVPCCluster() (admission.Warnings, error) { |
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.
Yes. But you can take that as an separate issue.
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.
May be for this PR scope you can consider making this check as first check in the function so we can fail fast.
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.
I can do that until it gets removed in favor of webhook validation then.
/ok-to-test |
cloud/scope/vpc_cluster.go
Outdated
|
||
// We must have an OperatingSystem value supplied in order to create the Custom Image. | ||
// NOTE(cjschaef): Perhaps we could try defaulting this value, so it isn't required for Custom Image creation. | ||
if s.IBMVPCCluster.Spec.Image.OperatingSystem == nil { |
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.
Yes. But you can take that as an separate issue.
} | ||
|
||
// NOTE: This tagging is only attempted once. We may wish to refactor in case this single attempt fails. | ||
if err := s.TagResource(s.Name(), *imageDetails.CRN); err != nil { |
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.
I can see you have handled this in ReconcileVPC, but here, you don't have a func like GetVPCID. You are checking either from status or from spec crn. If it's there you are validating the image and set the status based on that. Please take care of this here.
cloud/scope/vpc_cluster.go
Outdated
|
||
// No VPC Custom Image exists or was found. | ||
// So, check if the Image spec was defined, as it contains all the data necessary to reconcile. | ||
if s.IBMVPCCluster.Spec.Image == nil { |
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.
I have moved this check earlier, will be reflected once I push updates.
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.
For using this CustomImage for machine creation, will you be submitting another PR or planning to add to this as separate commit.
cloud/scope/vpc_cluster.go
Outdated
if s.IBMVPCCluster.Status.Image.ID != "" { | ||
imageID = ptr.To(s.IBMVPCCluster.Status.Image.ID) | ||
} else if s.IBMVPCCluster.Status.Image.Name != nil { | ||
image, err := s.VPCClient.GetImageByName(*s.IBMVPCCluster.Status.Image.Name) |
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.
Since we are only setting image status, Do you see any case where we set only Name not ID in status?
cloud/scope/vpc_cluster.go
Outdated
|
||
// We must have an OperatingSystem value supplied in order to create the Custom Image. | ||
// NOTE(cjschaef): Perhaps we could try defaulting this value, so it isn't required for Custom Image creation. | ||
if s.IBMVPCCluster.Spec.Image.OperatingSystem == nil { |
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.
May be for this PR scope you can consider making this check as first check in the function so we can fail fast.
cloud/scope/vpc_cluster.go
Outdated
image, err := s.createCustomImage() | ||
if err != nil { | ||
return false, fmt.Errorf("error failure trying to create vpc custom image: %w", err) | ||
} else if image == nil { |
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.
In createCustomImage() func, No where I see you are return nil, nil, So do you think this check is needed?
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.
I've refactored the function and return values, so this block has been removed (it will be reflected once I push up changes).
// buildCOSObjectHRef will build the HRef path to a COS Object that can be used for VPC Custom Image creation. | ||
func (s *VPCClusterScope) buildCOSObjectHRef() (*string, error) { | ||
// We need COS details in order to create the Custom Image from. | ||
if s.IBMVPCCluster.Spec.Image.COSInstance == nil || s.IBMVPCCluster.Spec.Image.COSBucket == nil || s.IBMVPCCluster.Spec.Image.COSObject == nil { |
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.
Lets create an issue or make a note to create a validation webhook for this, It will enhance user experience.
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.
I can create an issue.
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.
cloud/scope/vpc_cluster.go
Outdated
fileHRef, err := s.buildCOSObjectHRef() | ||
if err != nil { | ||
return nil, fmt.Errorf("error building vpc custom image file href: %w", err) | ||
} else if fileHRef == nil { |
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.
I think this check is not required.
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.
I will remove.
4e23720
to
2fb5b57
Compare
var imageID *string | ||
// Attempt to collect VPC Custom Image info from Status. | ||
if s.IBMVPCCluster.Status.Image != nil { | ||
if s.IBMVPCCluster.Status.Image.ID != "" { | ||
imageID = ptr.To(s.IBMVPCCluster.Status.Image.ID) | ||
} | ||
} else if s.IBMVPCCluster.Spec.Image == nil { | ||
// If no Image spec was defined, we expect it is maintained externally and continue without reconciling. For example, using a Catalog Offering Custom Image, which may be in another account, which means it cannot be looked up, but can be used when creating Instances. | ||
s.V(3).Info("No VPC Custom Image defined, skipping reconciliation") | ||
return false, nil | ||
} else if s.IBMVPCCluster.Spec.Image.Name != nil { | ||
// Attempt to retrieve the image details via the name, if it already exists | ||
imageDetails, err := s.VPCClient.GetImageByName(*s.IBMVPCCluster.Spec.Image.Name) | ||
if err != nil { | ||
return false, fmt.Errorf("error checking vpc custom image by name: %w", err) | ||
} else if imageDetails != nil && imageDetails.ID != nil { | ||
// Prevent relookup (API request) of VPC Custom Image if we already have the necessary data | ||
requeue := true | ||
if imageDetails.Status != nil && *imageDetails.Status == string(vpcv1.ImageStatusAvailableConst) { | ||
requeue = false | ||
} | ||
s.SetResourceStatus(infrav1beta2.ResourceTypeCustomImage, &infrav1beta2.ResourceStatus{ | ||
ID: *imageDetails.ID, | ||
Name: s.IBMVPCCluster.Spec.Image.Name, | ||
// Ready status will be invert of the need to requeue. | ||
Ready: !requeue, | ||
}) | ||
return requeue, nil | ||
} | ||
} else if s.IBMVPCCluster.Spec.Image.CRN != nil { | ||
// Parse the supplied Image CRN for Id, to perform image lookup. | ||
imageCRN, err := ParseCRN(*s.IBMVPCCluster.Spec.Image.CRN) | ||
if err != nil { | ||
return false, fmt.Errorf("error parsing vpc custom image crn: %w", err) | ||
} | ||
// If the value provided isn't a CRN or is missing the Resource ID, raise an error. | ||
if imageCRN == nil || imageCRN.Resource == "" { | ||
return false, fmt.Errorf("error parsing vpc custom image crn, missing resource id") | ||
} | ||
// If we didn't hit an error during parsing, and Resource was set, set that as the Image ID. | ||
imageID = ptr.To(imageCRN.Resource) | ||
} |
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.
var imageID *string | |
// Attempt to collect VPC Custom Image info from Status. | |
if s.IBMVPCCluster.Status.Image != nil { | |
if s.IBMVPCCluster.Status.Image.ID != "" { | |
imageID = ptr.To(s.IBMVPCCluster.Status.Image.ID) | |
} | |
} else if s.IBMVPCCluster.Spec.Image == nil { | |
// If no Image spec was defined, we expect it is maintained externally and continue without reconciling. For example, using a Catalog Offering Custom Image, which may be in another account, which means it cannot be looked up, but can be used when creating Instances. | |
s.V(3).Info("No VPC Custom Image defined, skipping reconciliation") | |
return false, nil | |
} else if s.IBMVPCCluster.Spec.Image.Name != nil { | |
// Attempt to retrieve the image details via the name, if it already exists | |
imageDetails, err := s.VPCClient.GetImageByName(*s.IBMVPCCluster.Spec.Image.Name) | |
if err != nil { | |
return false, fmt.Errorf("error checking vpc custom image by name: %w", err) | |
} else if imageDetails != nil && imageDetails.ID != nil { | |
// Prevent relookup (API request) of VPC Custom Image if we already have the necessary data | |
requeue := true | |
if imageDetails.Status != nil && *imageDetails.Status == string(vpcv1.ImageStatusAvailableConst) { | |
requeue = false | |
} | |
s.SetResourceStatus(infrav1beta2.ResourceTypeCustomImage, &infrav1beta2.ResourceStatus{ | |
ID: *imageDetails.ID, | |
Name: s.IBMVPCCluster.Spec.Image.Name, | |
// Ready status will be invert of the need to requeue. | |
Ready: !requeue, | |
}) | |
return requeue, nil | |
} | |
} else if s.IBMVPCCluster.Spec.Image.CRN != nil { | |
// Parse the supplied Image CRN for Id, to perform image lookup. | |
imageCRN, err := ParseCRN(*s.IBMVPCCluster.Spec.Image.CRN) | |
if err != nil { | |
return false, fmt.Errorf("error parsing vpc custom image crn: %w", err) | |
} | |
// If the value provided isn't a CRN or is missing the Resource ID, raise an error. | |
if imageCRN == nil || imageCRN.Resource == "" { | |
return false, fmt.Errorf("error parsing vpc custom image crn, missing resource id") | |
} | |
// If we didn't hit an error during parsing, and Resource was set, set that as the Image ID. | |
imageID = ptr.To(imageCRN.Resource) | |
} | |
if s.IBMVPCCluster.Spec.Image == nil { | |
// If no Image spec was defined, we expect it is maintained externally and continue without reconciling. For example, using a Catalog Offering Custom Image, which may be in another account, which means it cannot be looked up, but can be used when creating Instances. | |
s.V(3).Info("No VPC Custom Image defined, skipping reconciliation") | |
return false, nil | |
} else if s.IBMVPCCluster.Spec.Image.Name != nil { | |
// Attempt to retrieve the image details via the name, if it already exists | |
imageDetails, err := s.VPCClient.GetImageByName(*s.IBMVPCCluster.Spec.Image.Name) | |
if err != nil { | |
return false, fmt.Errorf("error checking vpc custom image by name: %w", err) | |
} else if imageDetails != nil && imageDetails.ID != nil { | |
// Prevent relookup (API request) of VPC Custom Image if we already have the necessary data | |
requeue := true | |
if imageDetails.Status != nil && *imageDetails.Status == string(vpcv1.ImageStatusAvailableConst) { | |
requeue = false | |
} | |
s.SetResourceStatus(infrav1beta2.ResourceTypeCustomImage, &infrav1beta2.ResourceStatus{ | |
ID: *imageDetails.ID, | |
Name: s.IBMVPCCluster.Spec.Image.Name, | |
// Ready status will be invert of the need to requeue. | |
Ready: !requeue, | |
}) | |
return requeue, nil | |
} | |
} | |
var imageID *string | |
// Attempt to collect VPC Custom Image info from Status. | |
if s.IBMVPCCluster.Status.Image != nil { | |
if s.IBMVPCCluster.Status.Image.ID != "" { | |
imageID = ptr.To(s.IBMVPCCluster.Status.Image.ID) | |
} | |
} else if s.IBMVPCCluster.Spec.Image.CRN != nil { | |
// Parse the supplied Image CRN for Id, to perform image lookup. | |
imageCRN, err := ParseCRN(*s.IBMVPCCluster.Spec.Image.CRN) | |
if err != nil { | |
return false, fmt.Errorf("error parsing vpc custom image crn: %w", err) | |
} | |
// If the value provided isn't a CRN or is missing the Resource ID, raise an error. | |
if imageCRN == nil || imageCRN.Resource == "" { | |
return false, fmt.Errorf("error parsing vpc custom image crn, missing resource id") | |
} | |
// If we didn't hit an error during parsing, and Resource was set, set that as the Image ID. | |
imageID = ptr.To(imageCRN.Resource) | |
} |
Can we please rearrange something like this?
Checking existing image based on name can be a separate block and using id to check the image status in a different block.
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.
No, I do not want to rearrange to check Spec prior to Status in this way.
I could move s.IBMVPCCluster.Spec.Image == nil
first, prior to Status check, but that would be the only check I'd want to move prior to Status check.
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.
Couple small changes, Otherwise LGTM. Thank you.
pkg/cloud/services/vpc/service.go
Outdated
} | ||
|
||
for i, v := range imagesList.Images { | ||
if (*v.Name) == imageName { |
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.
if (*v.Name) == imageName { | |
if *v.Name == imageName { |
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.
Sure, then I'd recommend the other Get*ByName
functions get updated later as well.
clusterScope.Info("Reconciling VPC Custom Image") | ||
if requeue, err := clusterScope.ReconcileVPCCustomImage(); err != nil { | ||
clusterScope.Error(err, "failed to reconcile VPC Custom Image") | ||
conditions.MarkFalse(clusterScope.IBMVPCCluster, infrav1beta2.ImageReadyCondition, infrav1beta2.VPCReconciliationFailedReason, capiv1beta1.ConditionSeverityError, err.Error()) |
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.
Lets define another reason specifically for CustomImage rather than using VPCReconciliationFailedReason
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.
oops, yes I will add a new reconciliation failed reason.
2fb5b57
to
9b28686
Compare
Add support to reconcile a VPC Custom Image for the new v2 VPC Infrastructure reconcile logic. Related: kubernetes-sigs#1896
9b28686
to
141d72f
Compare
LGTM! |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cjschaef, mkumatag The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Add support to reconcile a VPC Custom Image for the new v2 VPC Infrastructure reconcile logic.
Related: #1896
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
/area provider/ibmcloud
Release note: