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.

Sign up for GitHub

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

Jump to bottom

Add support for vm boot diagnostic profile #136

Merged
merged 4 commits into from
Mar 29, 2024
Merged

Add support for vm boot diagnostic profile #136

merged 4 commits into from
Mar 29, 2024

Conversation

hebelsan
Copy link
Contributor

What this PR does / why we need it:

  • Add support to enable vm boot diagnostics

Which issue(s) this PR fixes:
Fixes #104

Release note:

Machine-Controller-Manager Provider-Azure now supports enabling of vm boot diagnostics.

@hebelsan hebelsan requested review from a team as code owners March 18, 2024 12:35
@gardener-robot gardener-robot added needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Mar 18, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) 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 Mar 18, 2024
@rishabh-11 rishabh-11 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 19, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 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 Mar 19, 2024
@rishabh-11 rishabh-11 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 19, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 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 Mar 19, 2024
@hebelsan hebelsan mentioned this pull request Mar 20, 2024
Merged
@rishabh-11 rishabh-11 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 20, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 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 Mar 20, 2024
rishabh-11
rishabh-11 suggested changes Mar 20, 2024
View reviewed changes
Copy link
Contributor

@rishabh-11 rishabh-11 left a comment

Choose a reason for hiding this comment

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

Small changes needed. Rest looks good.

@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Mar 20, 2024
@rishabh-11 rishabh-11 self-assigned this Mar 20, 2024
@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 Mar 20, 2024
@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 Mar 20, 2024
@hebelsan
Copy link
Contributor Author

That's actually exactly how my first commit of this PR looked like :D
I thought it would be less complex and more talkative to always add the profile with default enabled=false, but I can see your point too.

@rishabh-11
Copy link
Contributor

One question @hebelsan. Did you test this PR by checking if this works as expected?

@hebelsan
Copy link
Contributor Author

@rishabh-11 yes, I've tested it manually.

@rishabh-11 rishabh-11 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 21, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 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 Mar 21, 2024
rishabh-11
rishabh-11 suggested changes Mar 26, 2024
View reviewed changes
Copy link
Contributor

@rishabh-11 rishabh-11 left a comment

Choose a reason for hiding this comment

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

can you add a validation for the diagnostics profile? If it is enabled, the storageURI cannot be empty, right?

@hebelsan
Copy link
Contributor Author

If storageURI us not specified azure managed storage will be used, which is actually the preferred option.
This is documented as comment in the providerSpec

rishabh-11
rishabh-11 approved these changes Mar 29, 2024
View reviewed changes
Copy link
Contributor

@rishabh-11 rishabh-11 left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/review Needs review labels Mar 29, 2024
@rishabh-11 rishabh-11 merged commit 1caac9d into gardener:master Mar 29, 2024
7 checks passed
@gardener-robot-ci-3 gardener-robot-ci-3 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 Mar 29, 2024
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rishabh-11 rishabh-11 rishabh-11 approved these changes

Assignees

@rishabh-11 rishabh-11

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 reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Optionally Enable boot diagnostics for Azure VM prior to startup
6 participants
@hebelsan @rishabh-11 @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3 @gardener-robot