Skip to content
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] Verify tags #507

Merged
merged 1 commit into from
Sep 24, 2020
Merged

[azure] Verify tags #507

merged 1 commit into from
Sep 24, 2020

Conversation

zuzzas
Copy link
Contributor

@zuzzas zuzzas commented Sep 7, 2020

What this PR does / why we need it:

Tag verification has been disabled back in May of 2019 by @prashanth26.

We can deduce that it was done due to Azure's unique ability to group resources in Resource Groups. Gardener project, presumably, packs all VMs (and relevant objects) into a single Resource Group (hence no need for tag verification).

That does not work for us, since we deploy master VMs near VMs that are managed by MCM and backed by Machine objects. It also seems weird that there are tags present in AzureMachineClass CRD, but are not used to verify actual objects.

Special notes for your reviewer:

Release note:

Restored tag verification in the Azure driver to filter VMs/disks/NICs based on tags

@zuzzas zuzzas requested review from ggaurav10 and a team as code owners September 7, 2020 10:50
@gardener-robot
Copy link

@zuzzas Thank you for your contribution.

@gardener-robot-ci-2
Copy link
Contributor

Thank you @zuzzas for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below.

@hardikdr hardikdr added this to the v0.35.0 milestone Sep 8, 2020
@hardikdr
Copy link
Member

hardikdr commented Sep 8, 2020

@zuzzas thanks for the PR, we'll take a look soon.

@hardikdr
Copy link
Member

/ok-to-test

@gardener-robot gardener-robot added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 10, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 10, 2020
@hardikdr
Copy link
Member

We can deduce that it was done due to Azure's unique ability to group resources in Resource Groups. Gardener project, presumably, packs all VMs (and relevant objects) into a single Resource Group (hence no need for tag verification).

Yes, though Azure helps in uniquely identifying the resources via resourceGroups, we still add tags to maintain the homogeneity across clouds. Also, tags on the VMs are specifically needed for certain aspects of Kubernetes to work properly, to identify if VMs belongs to the specific cluster or not, not sure if it's still the case, it was clearly sometime back.

Overall, the changes don't look breaking to me, I'd wait for comments from @prashanth26 @ggaurav10.

/lgtm otherwise.

cc @dkistner

@gardener-robot gardener-robot added the reviewed/lgtm Has approval for merging label Sep 14, 2020
@prashanth26
Copy link
Contributor

prashanth26 commented Sep 14, 2020

Tag verification has been disabled back in May of 2019 by @prashanth26.

This reason this was disabled back then was because we had observed Azure having bugs with tagging resources in the past and hence we stuck to just resource group-based filtering, I hope this doesn't affect us anymore.

cc: @amshuman-kr

@zuzzas
Copy link
Contributor Author

zuzzas commented Sep 14, 2020

@prashanth26
We've tested the proposed filtering on existing Azure clusters. Perhaps, it can have some adverse effects on large installations, but we haven't found any bugs yet.

@CLAassistant
Copy link

CLAassistant commented Sep 16, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@hardikdr hardikdr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@prashanth26
Copy link
Contributor

prashanth26 commented Sep 21, 2020

@prashanth26
We've tested the proposed filtering on existing Azure clusters. Perhaps, it can have some adverse effects on large installations, but we haven't found any bugs yet.

We ran into this corner case when we ended up with hundreds of orphan VMs/NICs had didn't seem to contain these tags anymore, although we had explicitly tagged them during creation. And this leads to these orphan VMs. Anyways I hope this doesn't occur anymore and we can merge this in and re-introduce this if we observe such issues again.

Copy link
Contributor

@prashanth26 prashanth26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm let's merge it in for now. If we see issues re-occurring we can take a second call later.

@hardikdr hardikdr added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 22, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 22, 2020
@hardikdr
Copy link
Member

hardikdr commented Sep 22, 2020

@zuzzas the check step is failing with, can you please correct it.


+ pull-request-gardener_machine-controller-manager-pr_master/.ci/check
15:57:56
Running go vet...
15:58:29
# github.com/gardener/machine-controller-manager/pkg/driver
15:58:29
pkg/driver/driver_azure.go:717:5: Infof format %q has arg server.Name of wrong type *string
15:58:29
pkg/driver/driver_azure.go:769:5: Infof format %q has arg nic.Name of wrong type *string
15:58:29
pkg/driver/driver_azure.go:825:6: Infof format %q has arg disk.Name of wrong type *string

Also, the unit-test is failing for the same reason, causing test step to fail.

Signed-off-by: Andrey Klimentyev <andrey.klimentyev@flant.com>
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 22, 2020
@zuzzas
Copy link
Contributor Author

zuzzas commented Sep 22, 2020

@hardikdr
That's what I get for trusting Goland IDE too much. Fixed!

@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 22, 2020
@hardikdr
Copy link
Member

The test step is now failing due to an issue with an internal CI cluster, will fix it soon, and merge.

@hardikdr
Copy link
Member

/ok-to-test

@gardener-robot gardener-robot added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 24, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 24, 2020
Copy link
Contributor

@prashanth26 prashanth26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@hardikdr hardikdr merged commit 4816c46 into gardener:master Sep 24, 2020
@zuzzas zuzzas deleted the azure-tags branch September 24, 2020 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants