OCPQE-26834 - GCP capi cases disks/ShieldedVms,Confidential VMs #341
Conversation
This file contains hidden or 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
sunzhaohua2
reviewed
Nov 20, 2024
sunzhaohua2
reviewed
Nov 20, 2024
sunzhaohua2
reviewed
Nov 20, 2024
sunzhaohua2
reviewed
Nov 20, 2024
miyadav
force-pushed
the
gcp-capi-cases-all
branch
from
4cae8ce to
a43f3c2
Compare
|
@sunzhaohua2 , here is the run with latest commits PTAL , when time permits , added all reviews |
miyadav
force-pushed
the
gcp-capi-cases-all
branch
from
a43f3c2 to
796143d
Compare
sunzhaohua2
reviewed
Nov 21, 2024
sunzhaohua2
reviewed
Nov 21, 2024
miyadav
force-pushed
the
gcp-capi-cases-all
branch
from
796143d to
0367e08
Compare
|
miyadav
force-pushed
the
gcp-capi-cases-all
branch
from
c5ff661 to
7cfd798
Compare
|
|
/test e2e-aws-capi-techpreview e2e-gcp-capi-techpreview e2e-aws-operator |
|
/test e2e-gcp-capi-techpreview |
miyadav
force-pushed
the
gcp-capi-cases-all
branch
5 times, most recently
from
72de761 to
b665e74
Compare
miyadav
force-pushed
the
gcp-capi-cases-all
branch
from
ada76a7 to
8defd12
Compare
|
@JoelSpeed could you take a look at this as well , if time permits.... |
JoelSpeed
reviewed
Jan 8, 2025
pkg/capi/gcp.go
Outdated
| var confidentialComputevalue = *createdTemplate.Spec.Template.Spec.ConfidentialCompute | ||
| Expect(fmt.Sprintf("%v", confidentialComputevalue)).To(Equal(fmt.Sprintf("%v", *confidentialCompute))) | ||
| }, | ||
| Entry("Confidential Compute enabled", &confidentialComputeEnabled), |
There was a problem hiding this comment.
You could use ptr.To rather than taking the pointer like this
From k8s.io/utils/ptr
We use it a lot in other tests
There was a problem hiding this comment.
I was having some trouble , with validation if I was using ptr.To so , I removed pointer value from compare to a simple string , will include that in next commit.
There was a problem hiding this comment.
Hmm, any idea what the issue was?
There was a problem hiding this comment.
value of *confidentialCompute is Enabled or Disabled I thought return value of ptr.To was boolean true/false whereas it can be pointer to any literal , will modify the code as below -
expectedConfidentialComputeValue := ptr.To(
map[bool]string{
true: "Enabled",
false: "Disabled",
}[*confidentialCompute == gcpv1.ConfidentialComputePolicyEnabled],
)
Expect(fmt.Sprintf("%v", *expectedConfidentialComputeValue)).To(Equal(fmt.Sprintf("%v", *confidentialCompute)))
8defd12 to
eb84fb4
Compare
|
/test e2e-gcp-capi-techpreview |
damdo
reviewed
Jan 9, 2025
miyadav
force-pushed
the
gcp-capi-cases-all
branch
from
eb84fb4 to
1da2322
Compare
|
@damdo @JoelSpeed made the changes( templatename generation) , validated changes . Should be good . PTAL , when time permits , will trigger e2e as well. |
JoelSpeed
reviewed
Jan 9, 2025
pkg/capi/gcp.go
Outdated
| var confidentialComputevalue = *createdTemplate.Spec.Template.Spec.ConfidentialCompute | ||
| Expect(fmt.Sprintf("%v", confidentialComputevalue)).To(Equal(fmt.Sprintf("%v", *confidentialCompute))) | ||
| }, | ||
| Entry("Confidential Compute enabled", &confidentialComputeEnabled), |
There was a problem hiding this comment.
Hmm, any idea what the issue was?
miyadav
force-pushed
the
gcp-capi-cases-all
branch
from
1da2322 to
0cef15b
Compare
JoelSpeed
reviewed
Jan 10, 2025
pkg/capi/gcp.go
Outdated
Comment on lines
146
to
191
| // Define constants as variable for this case to pass values properly | ||
| confidentialComputeEnabled := gcpv1.ConfidentialComputePolicyEnabled | ||
| confidentialComputeDisabled := gcpv1.ConfidentialComputePolicyDisabled | ||
| DescribeTable("should configure Confidential VM correctly", framework.LabelCAPI, framework.LabelDisruptive, | ||
| func(confidentialCompute *gcpv1.ConfidentialComputePolicy) { | ||
| mapiProviderSpec := getGCPMAPIProviderSpec(cl) | ||
| Expect(mapiProviderSpec).ToNot(BeNil()) | ||
| gcpMachineTemplate = createGCPMachineTemplate(mapiProviderSpec) | ||
| gcpMachineTemplate.Spec.Template.Spec.ConfidentialCompute = confidentialCompute | ||
| gcpMachineTemplate.Spec.Template.Spec.InstanceType = "n2d-standard-4" | ||
|
|
||
| if *confidentialCompute == gcpv1.ConfidentialComputePolicyEnabled { | ||
| mapiProviderSpec.OnHostMaintenance = OnHostMaintenanceTerminate | ||
| gcpMachineTemplate.Spec.Template.Spec.OnHostMaintenance = (*gcpv1.HostMaintenancePolicy)(&mapiProviderSpec.OnHostMaintenance) | ||
| } else { | ||
| mapiProviderSpec.OnHostMaintenance = "Migrate" | ||
| gcpMachineTemplate.Spec.Template.Spec.OnHostMaintenance = (*gcpv1.HostMaintenancePolicy)(&mapiProviderSpec.OnHostMaintenance) | ||
| } | ||
|
|
||
| Expect(cl.Create(ctx, gcpMachineTemplate)).To(Succeed()) | ||
|
|
||
| By("Creating a MachineSet for Confidential VM") | ||
| machineSet, err = framework.CreateCAPIMachineSet(ctx, cl, framework.NewCAPIMachineSetParams( | ||
| "gcp-machineset-confidential-74703", | ||
| clusterName, | ||
| mapiProviderSpec.Zone, | ||
| 1, | ||
| corev1.ObjectReference{ | ||
| Kind: "GCPMachineTemplate", | ||
| APIVersion: infraAPIVersion, | ||
| Name: gcpMachineTemplate.Name, | ||
| }, | ||
| )) | ||
| Expect(err).ToNot(HaveOccurred(), "Failed to create CAPI MachineSet with Confidential VM configuration") | ||
|
|
||
| framework.WaitForCAPIMachinesRunning(ctx, cl, machineSet.Name) | ||
|
|
||
| By("Verifying the Confidential VM configuration on the created GCP MachineTemplate") | ||
| createdTemplate := &gcpv1.GCPMachineTemplate{} | ||
| Expect(cl.Get(framework.GetContext(), client.ObjectKey{ | ||
| Namespace: framework.ClusterAPINamespace, | ||
| Name: gcpMachineTemplate.Name, | ||
| }, createdTemplate)).To(Succeed()) | ||
|
|
||
| expectedConfidentialComputeValue := ptr.To( | ||
| map[bool]string{ | ||
| true: "Enabled", | ||
| false: "Disabled", | ||
| }[*confidentialCompute == gcpv1.ConfidentialComputePolicyEnabled], | ||
| ) | ||
|
|
||
| Expect(fmt.Sprintf("%v", *expectedConfidentialComputeValue)).To(Equal(fmt.Sprintf("%v", *confidentialCompute))) | ||
|
|
||
| }, | ||
| Entry("Confidential Compute enabled", &confidentialComputeEnabled), | ||
| Entry("Confidential Compute disabled", &confidentialComputeDisabled), | ||
| ) |
There was a problem hiding this comment.
I think this can be simplified if we don't use fewer pointers
Suggested change
| // Define constants as variable for this case to pass values properly | |
| confidentialComputeEnabled := gcpv1.ConfidentialComputePolicyEnabled | |
| confidentialComputeDisabled := gcpv1.ConfidentialComputePolicyDisabled | |
| DescribeTable("should configure Confidential VM correctly", framework.LabelCAPI, framework.LabelDisruptive, | |
| func(confidentialCompute *gcpv1.ConfidentialComputePolicy) { | |
| mapiProviderSpec := getGCPMAPIProviderSpec(cl) | |
| Expect(mapiProviderSpec).ToNot(BeNil()) | |
| gcpMachineTemplate = createGCPMachineTemplate(mapiProviderSpec) | |
| gcpMachineTemplate.Spec.Template.Spec.ConfidentialCompute = confidentialCompute | |
| gcpMachineTemplate.Spec.Template.Spec.InstanceType = "n2d-standard-4" | |
| if *confidentialCompute == gcpv1.ConfidentialComputePolicyEnabled { | |
| mapiProviderSpec.OnHostMaintenance = OnHostMaintenanceTerminate | |
| gcpMachineTemplate.Spec.Template.Spec.OnHostMaintenance = (*gcpv1.HostMaintenancePolicy)(&mapiProviderSpec.OnHostMaintenance) | |
| } else { | |
| mapiProviderSpec.OnHostMaintenance = "Migrate" | |
| gcpMachineTemplate.Spec.Template.Spec.OnHostMaintenance = (*gcpv1.HostMaintenancePolicy)(&mapiProviderSpec.OnHostMaintenance) | |
| } | |
| Expect(cl.Create(ctx, gcpMachineTemplate)).To(Succeed()) | |
| By("Creating a MachineSet for Confidential VM") | |
| machineSet, err = framework.CreateCAPIMachineSet(ctx, cl, framework.NewCAPIMachineSetParams( | |
| "gcp-machineset-confidential-74703", | |
| clusterName, | |
| mapiProviderSpec.Zone, | |
| 1, | |
| corev1.ObjectReference{ | |
| Kind: "GCPMachineTemplate", | |
| APIVersion: infraAPIVersion, | |
| Name: gcpMachineTemplate.Name, | |
| }, | |
| )) | |
| Expect(err).ToNot(HaveOccurred(), "Failed to create CAPI MachineSet with Confidential VM configuration") | |
| framework.WaitForCAPIMachinesRunning(ctx, cl, machineSet.Name) | |
| By("Verifying the Confidential VM configuration on the created GCP MachineTemplate") | |
| createdTemplate := &gcpv1.GCPMachineTemplate{} | |
| Expect(cl.Get(framework.GetContext(), client.ObjectKey{ | |
| Namespace: framework.ClusterAPINamespace, | |
| Name: gcpMachineTemplate.Name, | |
| }, createdTemplate)).To(Succeed()) | |
| expectedConfidentialComputeValue := ptr.To( | |
| map[bool]string{ | |
| true: "Enabled", | |
| false: "Disabled", | |
| }[*confidentialCompute == gcpv1.ConfidentialComputePolicyEnabled], | |
| ) | |
| Expect(fmt.Sprintf("%v", *expectedConfidentialComputeValue)).To(Equal(fmt.Sprintf("%v", *confidentialCompute))) | |
| }, | |
| Entry("Confidential Compute enabled", &confidentialComputeEnabled), | |
| Entry("Confidential Compute disabled", &confidentialComputeDisabled), | |
| ) | |
| DescribeTable("should configure Confidential VM correctly", framework.LabelCAPI, framework.LabelDisruptive, | |
| func(confidentialCompute gcpv1.ConfidentialComputePolicy) { | |
| mapiProviderSpec := getGCPMAPIProviderSpec(cl) | |
| Expect(mapiProviderSpec).ToNot(BeNil()) | |
| gcpMachineTemplate = createGCPMachineTemplate(mapiProviderSpec) | |
| gcpMachineTemplate.Spec.Template.Spec.ConfidentialCompute = ptr.To(confidentialCompute) | |
| gcpMachineTemplate.Spec.Template.Spec.InstanceType = "n2d-standard-4" | |
| switch confidentialCompute { | |
| case gcpv1.ConfidentialComputePolicyEnabled: | |
| mapiProviderSpec.OnHostMaintenance = OnHostMaintenanceTerminate | |
| gcpMachineTemplate.Spec.Template.Spec.OnHostMaintenance = ptr.To(gcpv1.HostMaintenancePolicy(mapiProviderSpec.OnHostMaintenance)) | |
| case gcpv1.ConfidentialComputePolicyDisabled: | |
| mapiProviderSpec.OnHostMaintenance = "Migrate" | |
| gcpMachineTemplate.Spec.Template.Spec.OnHostMaintenance = ptr.To(gcpv1.HostMaintenancePolicy(mapiProviderSpec.OnHostMaintenance)) | |
| } | |
| Expect(cl.Create(ctx, gcpMachineTemplate)).To(Succeed()) | |
| By("Creating a MachineSet for Confidential VM") | |
| machineSet, err = framework.CreateCAPIMachineSet(ctx, cl, framework.NewCAPIMachineSetParams( | |
| "gcp-machineset-confidential-74703", | |
| clusterName, | |
| mapiProviderSpec.Zone, | |
| 1, | |
| corev1.ObjectReference{ | |
| Kind: "GCPMachineTemplate", | |
| APIVersion: infraAPIVersion, | |
| Name: gcpMachineTemplate.Name, | |
| }, | |
| )) | |
| Expect(err).ToNot(HaveOccurred(), "Failed to create CAPI MachineSet with Confidential VM configuration") | |
| framework.WaitForCAPIMachinesRunning(ctx, cl, machineSet.Name) | |
| By("Verifying the Confidential VM configuration on the created GCP MachineTemplate") | |
| createdTemplate := &gcpv1.GCPMachineTemplate{} | |
| Expect(cl.Get(framework.GetContext(), client.ObjectKey{ | |
| Namespace: framework.ClusterAPINamespace, | |
| Name: gcpMachineTemplate.Name, | |
| }, createdTemplate)).To(Succeed()) | |
| Expect(createdTemplate.Spec.Template.Spec.ConfidentialCompute).To(HaveValue(Equal(confidentialCompute))) | |
| }, | |
| Entry("Confidential Compute enabled", gcpv1.ConfidentialComputePolicyEnabled), | |
| Entry("Confidential Compute disabled", gcpv1.ConfidentialComputePolicyDisabled), | |
| ) |
pkg/capi/gcp.go
Outdated
Comment on lines
190
to
197
| expectedConfidentialComputeValue := ptr.To( | ||
| map[bool]string{ | ||
| true: "Enabled", | ||
| false: "Disabled", | ||
| }[*confidentialCompute == gcpv1.ConfidentialComputePolicyEnabled], | ||
| ) | ||
|
|
||
| Expect(fmt.Sprintf("%v", *expectedConfidentialComputeValue)).To(Equal(fmt.Sprintf("%v", *confidentialCompute))) |
There was a problem hiding this comment.
I'm not really sure what/why we are testing this? This appears to just be checking the value of the input to the test? Should it not instead be comparing the value from the template itself?
We are creating the CAPI template, so why are we checking the values on the template?
There was a problem hiding this comment.
Earlier was using the template to compare missed it in latest commit , I will go through the code you suggested , will commit that
var confidentialComputevalue = *createdTemplate.Spec.Template.Spec.ConfidentialCompute
Expect(fmt.Sprintf("%v", confidentialComputevalue)).To(Equal(fmt.Sprintf("%v", *confidentialCompute)))
}
| confidentialComputeDisabled := gcpv1.ConfidentialComputePolicyDisabled | ||
| DescribeTable("should configure Confidential VM correctly", framework.LabelCAPI, framework.LabelDisruptive, | ||
| func(confidentialCompute *gcpv1.ConfidentialComputePolicy) { | ||
| mapiProviderSpec := getGCPMAPIProviderSpec(cl) |
There was a problem hiding this comment.
What is this providerSpec used for? We are setting the onhostmaintenance stuff, but we don't actually apply it to the cluster?
There was a problem hiding this comment.
We apply it when ConfidentialCompute is enabled.
There was a problem hiding this comment.
I don't see that? You're creating a GCP Machine template, and a GCP MachineSet, I don't see you using a MAPI providerSpec anywhere as you aren't creating MAPI machines?
There was a problem hiding this comment.
yes Joel you are correct , but populating values for gcpmachinetemplate eg- zone with machineproviderspec.
so I was making changes to confidentialcompute fields and using the rest of the values to make the template similar to machineproviderspec from mapi , this was done on all tests.
There was a problem hiding this comment.
Right I see what's going on here, but then why do you modify the mapiProviderSpec after line 150?
Should line 150 come after the switch that updates the MAPI provider spec?
There was a problem hiding this comment.
so does making below change is same instead of moving 150 down , I can do below? You feel, it might be redundant code at this moment ? I am not sure , how I reached there 🤔
switch confidentialCompute {
case gcpv1.ConfidentialComputePolicyEnabled:
gcpMachineTemplate.Spec.Template.Spec.OnHostMaintenance = ptr.To(gcpv1.HostMaintenancePolicy("onHostMaintenance"))
case gcpv1.ConfidentialComputePolicyDisabled:
gcpMachineTemplate.Spec.Template.Spec.OnHostMaintenance = ptr.To(gcpv1.HostMaintenancePolicy("Migrate"))
}
There was a problem hiding this comment.
Yeah, I think the switch is redundant. You convert the MAPI provider spec to the GCP Template spec on 150, after that you don't re-use the mapi provider spec so any further changes aren't being considered. I would have expected all that setup to come ahead of the conversion
There was a problem hiding this comment.
If I try to do that afterwards , it will result in panic due to gcpmachinetemplate being empty ?
There was a problem hiding this comment.
Yes, my expectation was to move the setting of any of the gcpMachineTemplate into the conversion function createGCPMachineTemplate, I see in most tests you're modifying the gcpMachineTemplate after you've got the result of that function.
Either you need to modify the mapi, or the capi one, if you modify the mapi one, then do that before you pass it into createGCPMachineTemplate and ensure that createGCPMachineTemplate handles all the fields you're setting.
Or remove modifications to mapiProviderSpec after you call createGCPMachineTemplate, those modifications are not being read apart from in the lines directly after them, in which case you may as well cut out the middle step no?
Refractored to follow Table structure for all cases comment added for disk cases(author/casenumber) Refractored to use create func effieciently Refractored confidentialVMs case ( By Joel ) Added test for preemptible and GPU instances refractor for onHost constant values
miyadav
force-pushed
the
gcp-capi-cases-all
branch
from
0cef15b to
e470af1
Compare
Added lables and removed comments e2 instances do not support onHostMaintenance=TERMINATE Removed GPU case for now generatename for templates update to skip clean up if no resources were created
miyadav
force-pushed
the
gcp-capi-cases-all
branch
from
e470af1 to
597a7c1
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed 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 |
openshift-ci
bot
added
the
approved
1 similar comment
|
/retest-required |
|
@miyadav: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
openshift-merge-bot
bot
merged commit e0af696
into
openshift:master
7 of 10 checks passed
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.
@sunzhaohua2 @huali9 @RadekManak @damdo @racheljpg PTAL when time permits,
All tests works well .. Added some info in comments for few scenarios.