CFE-1051: Add the webhook validation for "CapacityReservationGroupID" to "AzureMachineProviderSpec" in openshift/machine-api-operator #1234

anirudhAgniRedhat · 2024-04-30T09:33:53Z

add the webhook validation for the "CapacityReservationGroupID" field of "AzureMachineProviderSpec" in openshift/machine-api-operator so that Azure capacity reservation can be supported

This is follow-up PR for openshift/api#1866

I have to update the go.mod file to replace openshift/api dependency as the api PR is not merged yet.
I will update the vendor and go.mod file once the reviewers are good with the implementations.
we can merge API PR and remove dependency.

Signed-off-by: anirudhAgniRedhat <aagnihot@redhat.com>

openshift-ci-robot · 2024-04-30T09:33:57Z

@anirudhAgniRedhat: This pull request references CFE-1051 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

add the webhook validation for the "CapacityReservationGroupID" field of "AzureMachineProviderSpec" in openshift/machine-api-operator so that Azure capacity reservation can be supported

This is follow-up PR for openshift/api#1866

I have to update the go.mod file to replace openshift/api dependency as the api PR is not merged yet.
I will update the vendor and go.mod file once the reviewers are good with the implementations.
we can merge API PR and remove dependency.

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 openshift-eng/jira-lifecycle-plugin repository.

JoelSpeed · 2024-04-30T13:40:42Z

This is looking good code wise, but we should be able to get the unit and crds sync checks green, can you take a look at why those are currently not passing?

anirudhAgniRedhat · 2024-05-01T18:06:33Z

This is looking good code wise, but we should be able to get the unit and crds sync checks green, can you take a look at why those are currently not passing?

@JoelSpeed I checked the tests are failing because I have updated the go.mod file by adding replace github.com/openshift/api => github.com/anirudhAgniRedhat/openshift-api v0.0.2 and running go mod tidy and go mod vendor

The error which has emerged as due to updating vendor that brings the latest dependency changed in openshift/api repository.

The same is the reason for failing tests in openshift/machine-api-provider-azure#107

=== RUN   TestMachineEvents
I0501 22:51:16.812722   14450 session.go:91] No existing vCenter session found, creating new session
    actuator_test.go:162: 
        Expected success, but got an error:
            <*meta.NoKindMatchError | 0xc000dafd40>: 
            no matches for kind "Infrastructure" in version "config.openshift.io/v1"
            {
                GroupKind: {
                    Group: "config.openshift.io",
                    Kind: "Infrastructure",
                },
                SearchedVersions: ["v1"],
            }
--- FAIL: TestMachineEvents (4.27s)

=== RUN   TestPatchMachine
I0501 22:51:22.937439   14450 session.go:91] No existing vCenter session found, creating new session
    machine_scope_test.go:381: 
        Expected success, but got an error:
            <*meta.NoKindMatchError | 0xc0008fa8c0>: 
            no matches for kind "Infrastructure" in version "config.openshift.io/v1"
            {
                GroupKind: {
                    Group: "config.openshift.io",
                    Kind: "Infrastructure",
                },
                SearchedVersions: ["v1"],
            }
--- FAIL: TestPatchMachine (3.23s)

Regarding pull-ci-openshift-machine-api-operator-master-verify-crds-sync it is searching for vendor/github.com/openshift/api/machine/v1beta1/0000_10_machine.crd.yaml but could not find the file in updated vendor directory thats why it is giving me error as cp: cannot stat 'vendor/github.com/openshift/api/machine/v1beta1/0000_10_machine.crd.yaml': No such file or directory

Can you suggest any fix for this?
I guess after merging openshift/api#1866 and upgrading versions could fix this?
Need your review over this.

JoelSpeed · 2024-05-07T09:38:13Z

pkg/webhooks/machine_webhook.go

@@ -9,6 +9,7 @@ import (
 	"strconv"
 	"strings"

+	_ "github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests"


This goes in the tools.go file

JoelSpeed · 2024-05-07T14:19:43Z

pkg/controller/vsphere/actuator_test.go

@@ -62,7 +62,7 @@ func TestMachineEvents(t *testing.T) {
 	testEnv := &envtest.Environment{
 		CRDDirectoryPaths: []string{
 			filepath.Join("..", "..", "..", "install"),
-			filepath.Join("..", "..", "..", "vendor", "github.com", "openshift", "api", "config", "v1"),
+			filepath.Join("..", "..", "..", "tools", "vendor", "github.com", "openshift", "api", "config", "v1", "zz_generated.crd-manifests"),


Didn't expect this to be in tools, why do we have 2 tools.go in this repo, odd 🤔

JoelSpeed · 2024-05-07T14:20:36Z

tools/tools.go

+	_ "github.com/openshift/api/config/v1/zz_generated.crd-manifests"
+	_ "github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests"


Move these to the root tools.go not this version, not sure why they are separate but it will be cleaner in that top level one. It will remove the need to have two o/api dependencies that could drift

This reverts commit f7ecd28.

JoelSpeed · 2024-05-07T16:52:45Z

Changes LGTM bar the replace statement for the API dep, so lets try get this tested with QE and the other 2 PRs

JoelSpeed · 2024-05-13T15:23:49Z

pkg/webhooks/machine_webhook.go

+		if providerSpec.CapacityReservationGroupID != "" && !validateAzureImmutabilityForCapacityReservationGroupID(oldProviderSpec.CapacityReservationGroupID, providerSpec.CapacityReservationGroupID) {
+			errs = append(errs, field.Invalid(field.NewPath("providerSpec", "capacityReservationGroupID"),
+				providerSpec.CapacityReservationGroupID, "capacityReservationGroupID is immutable"))
+		}


Will this execute on only Machines or also MachineSets?

We don't typically validate immutability inside providerSpecs since the type is re-used across the Machines and MachineSets and I don't believe we distinguish between the two when this validation code is called

@JoelSpeed This is used in both machine and machineset

It shouldn't validate machinesets?

We don't want to validate this as immutable here, since it would affect both machines and machinesets. Lets drop this. With most fields in the machine provider spec, they can be updated, but take no effect, this is standard

So should we remove the immutability validation from everywhere? or this validation should be added somewhere else?

Only downstream, for this feature yes. Where else is it?

it is written in openshift/api#1866 in godocs should be remove it from there as well.

TrilokGeer · 2024-05-14T09:44:34Z

pkg/controller/vsphere/util.go

@@ -22,7 +22,7 @@ import (

 const (
 	globalInfrastuctureName  = "cluster"
-	openshiftConfigNamespace = "openshift-config"
+	openshiftConfigNamespace = "openshift-config-1"


What is significance of this new namespace?

@TrilokGeer Have to update this because the existing suite test was failing because the old namespace is forbidden to delete.

This reverts commit 41c1c37.

anirudhAgniRedhat · 2024-05-14T11:01:41Z

@JoelSpeed I have reverted the commit for immutability
As the current code architecture is not allowing to add the the immuatbility validation inside providerSpec for machine only.

JoelSpeed

/approve
/lgtm

openshift-ci · 2024-05-22T13:09:37Z

[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:

~~OWNERS~~ [JoelSpeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2024-05-22T15:19:04Z

@anirudhAgniRedhat: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-metal-ipi-ovn-dualstack	`31e2a4b`	link	false	`/test e2e-metal-ipi-ovn-dualstack`
ci/prow/e2e-azure-operator	`31e2a4b`	link	false	`/test e2e-azure-operator`
ci/prow/e2e-vsphere-operator	`31e2a4b`	link	false	`/test e2e-vsphere-operator`
ci/prow/e2e-vsphere-ovn	`31e2a4b`	link	false	`/test e2e-vsphere-ovn`
ci/prow/e2e-vsphere-ovn-upgrade	`31e2a4b`	link	false	`/test e2e-vsphere-ovn-upgrade`
ci/prow/e2e-vsphere-ovn-serial	`31e2a4b`	link	false	`/test e2e-vsphere-ovn-serial`

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-bot · 2024-05-22T20:58:53Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-machine-api-operator-container-v4.17.0-202405221910.p0.ge5c34fe.assembly.stream.el9 for distgit ose-machine-api-operator.
All builds following this will include this PR.

…udhAgniRedhat/capacityReservationGroupIDWebhookValidation" This reverts commit e5c34fe, reversing changes made to dcf1387.

https://issues.redhat.com/browse/OCPBUGS-34391: Revert #1234 "CFE-1051: Add the webhook validation for \"CapacityReservationGroupID\" to \"AzureMachineProviderSpec\" in openshift/machine-api-operator"

…ift#1234 "CFE-1051: Add the webhook validation for \"CapacityReservationGroupID\" to \"AzureMachineProviderSpec\" in openshift/machine-api-operator""

Added Validation for webhook

a207137

Signed-off-by: anirudhAgniRedhat <aagnihot@redhat.com>

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 30, 2024

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 30, 2024

openshift-ci bot requested review from JoelSpeed and racheljpg April 30, 2024 09:36

anirudhAgniRedhat mentioned this pull request Apr 30, 2024

CFE-1050: Add support of capacity reservation group in openshift/machine-api-provider-azure openshift/machine-api-provider-azure#107

Merged

Added Tests

0e5c138

JoelSpeed reviewed May 7, 2024

View reviewed changes

anirudhAgniRedhat added 3 commits May 7, 2024 18:04

Fixes CI tests

f163bd4

Fixed tests

2196939

Added tools.go dependency

f7ecd28

JoelSpeed reviewed May 7, 2024

View reviewed changes

anirudhAgniRedhat added 2 commits May 7, 2024 20:11

Revert "Added tools.go dependency"

b871672

This reverts commit f7ecd28.

Added Dependency in tools.go

a779f91

Added immutability validations

41c1c37

JoelSpeed reviewed May 13, 2024

View reviewed changes

TrilokGeer reviewed May 14, 2024

View reviewed changes

Revert "Added immutability validations"

210f14a

This reverts commit 41c1c37.

anirudhAgniRedhat added 2 commits May 22, 2024 00:37

updated go.mod

45a303d

Added api/feature directory update

31e2a4b

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2024

JoelSpeed reviewed May 22, 2024

View reviewed changes

openshift-ci bot assigned JoelSpeed May 22, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 22, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 22, 2024

openshift-merge-bot bot merged commit e5c34fe into openshift:master May 22, 2024
20 of 26 checks passed

vr4manta mentioned this pull request May 23, 2024

SPLAT-1638: Added multi vCenter support #1241

Open

dgoodwin mentioned this pull request May 23, 2024

https://issues.redhat.com/browse/OCPBUGS-34391: Revert #1234 "CFE-1051: Add the webhook validation for \"CapacityReservationGroupID\" to \"AzureMachineProviderSpec\" in openshift/machine-api-operator" #1246

Merged

DennisPeriquet mentioned this pull request May 23, 2024

TRT-1681: Revert #1234 "CFE-1051: Add the webhook validation for \"CapacityReservationGroupID\" to \"AzureMachineProviderSpec\" in openshift/machine-api-operator" #1248

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CFE-1051: Add the webhook validation for "CapacityReservationGroupID" to "AzureMachineProviderSpec" in openshift/machine-api-operator #1234

CFE-1051: Add the webhook validation for "CapacityReservationGroupID" to "AzureMachineProviderSpec" in openshift/machine-api-operator #1234

anirudhAgniRedhat commented Apr 30, 2024

openshift-ci-robot commented Apr 30, 2024 •

edited by openshift-ci bot

JoelSpeed commented Apr 30, 2024

anirudhAgniRedhat commented May 1, 2024 •

edited

JoelSpeed May 7, 2024

JoelSpeed May 7, 2024

JoelSpeed May 7, 2024

JoelSpeed commented May 7, 2024

JoelSpeed May 13, 2024

anirudhAgniRedhat May 13, 2024

anirudhAgniRedhat May 14, 2024

JoelSpeed May 14, 2024

anirudhAgniRedhat May 14, 2024 •

edited

JoelSpeed May 14, 2024

anirudhAgniRedhat May 14, 2024

TrilokGeer May 14, 2024

anirudhAgniRedhat May 14, 2024 •

edited

anirudhAgniRedhat commented May 14, 2024 •

edited

JoelSpeed left a comment

openshift-ci bot commented May 22, 2024

openshift-ci bot commented May 22, 2024

openshift-bot commented May 22, 2024

		_ "github.com/openshift/api/config/v1/zz_generated.crd-manifests"
		_ "github.com/openshift/api/machine/v1beta1/zz_generated.crd-manifests"

CFE-1051: Add the webhook validation for "CapacityReservationGroupID" to "AzureMachineProviderSpec" in openshift/machine-api-operator #1234

CFE-1051: Add the webhook validation for "CapacityReservationGroupID" to "AzureMachineProviderSpec" in openshift/machine-api-operator #1234

Conversation

anirudhAgniRedhat commented Apr 30, 2024

openshift-ci-robot commented Apr 30, 2024 • edited by openshift-ci bot

JoelSpeed commented Apr 30, 2024

anirudhAgniRedhat commented May 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anirudhAgniRedhat May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anirudhAgniRedhat May 14, 2024 • edited

Choose a reason for hiding this comment

anirudhAgniRedhat commented May 14, 2024 • edited

JoelSpeed left a comment

Choose a reason for hiding this comment

openshift-ci bot commented May 22, 2024

openshift-ci bot commented May 22, 2024

openshift-bot commented May 22, 2024

openshift-ci-robot commented Apr 30, 2024 •

edited by openshift-ci bot

anirudhAgniRedhat commented May 1, 2024 •

edited

anirudhAgniRedhat May 14, 2024 •

edited

anirudhAgniRedhat May 14, 2024 •

edited

anirudhAgniRedhat commented May 14, 2024 •

edited