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

support extended resources in providerConfig.nodeTemplate, inherit core resources from pool.nodeTemplate #1010

Merged
merged 6 commits into from
Nov 14, 2024
Merged

support extended resources in providerConfig.nodeTemplate, inherit core resources from pool.nodeTemplate #1010

merged 6 commits into from
Nov 14, 2024

Conversation

elankath
Copy link
Contributor

How to categorize this PR?

/area auto-scaling
/kind enhancement
/platform aws

What this PR does / why we need it:

Permit specification of custom extended resources in the worker.providerConfig.nodeTemplate without needing to explicitly re-specify core resources such as cpu, gpu, memory for the machine type again.

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

Special notes for your reviewer:

Release note:

Support specification of extended resources in provider config node template without re-specifying core resources.

@elankath elankath requested review from a team as code owners July 18, 2024 05:53
@gardener-robot gardener-robot added needs/review Needs review area/auto-scaling Auto-scaling (CA/HPA/VPA/HVPA, predominantly control plane, but also otherwise) related kind/enhancement Enhancement, improvement, extension platform/aws Amazon web services platform/infrastructure labels Jul 18, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 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 Jul 18, 2024
@gardener-robot gardener-robot added the size/s Size of pull request is small (see gardener-robot robot/bots/size.py) label Jul 18, 2024
@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 Jul 18, 2024
@elankath
Copy link
Contributor Author

elankath commented Jul 23, 2024 •
edited
Loading

@kon-angelo could you review this ? I would also like add additional unit test(s). But I need the unit test to check workerDelegate.machineClasses to see if extended resources have been set in. Unfortunately, it is un-exported field in the workerDelegate. Is it OK to introduce a method to expose this ? (Inside pkg/controller/worker/actuator.go) ?

@elankath
Copy link
Contributor Author

elankath commented Sep 4, 2024

@voelzmo this PR will be updated to latest changes and feedback next week and sent for re-review.

@gardener-robot gardener-robot added the needs/rebase Needs git rebase label Oct 15, 2024
@gardener-robot
Copy link

@elankath You need rebase this pull request with latest master branch. Please check.

@elankath
Copy link
Contributor Author

elankath commented Oct 15, 2024 •
edited
Loading

This PR is now re-based with latest changes and now simplified. Sorry for delay.

@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 Nov 7, 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 Nov 7, 2024
@elankath
Copy link
Contributor Author

elankath commented Nov 7, 2024

Re-based with latest changed and simplified.

@elankath elankath changed the title support extended resources without re-specifying core resources support extended resources in providerConfig.nodeTemplate, inherit missing resources from pool.nodeTemplate Nov 7, 2024
@elankath elankath changed the title support extended resources in providerConfig.nodeTemplate, inherit missing resources from pool.nodeTemplate support extended resources in providerConfig.nodeTemplate, inherit core resources from pool.nodeTemplate Nov 7, 2024
@elankath
Copy link
Contributor Author

elankath commented Nov 7, 2024 •
edited
Loading

@kon-angelo I would like to make worker.workerDelegate type as exported ie make it worker.WorkerDelegate to enable unit-test that checks machineClasses inside this struct type after workerDelegate.GenerateMachineDeployments() is invoked.. The problem is that our test code is in a different package worker_test and hence cannot accerss workerDelegate. Please give me a go to do this and I will add additional unit-tests for this PR after making this type exported.

@gardener-robot gardener-robot added size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) and removed size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels Nov 7, 2024
@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 Nov 7, 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 Nov 7, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 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 Nov 7, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 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 Nov 7, 2024
@elankath
Copy link
Contributor Author

elankath commented Nov 12, 2024 •
edited
Loading

/assign @kon-angelo
/assign @MartinWeindel

Can you take a look to see if this is OK or requires changes ?

@elankath
Copy link
Contributor Author

/assign @kon-angelo
/assign @MartinWeindel

@AndreasBurger AndreasBurger merged commit 9b57681 into gardener:master Nov 14, 2024
10 checks passed
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndreasBurger AndreasBurger AndreasBurger approved these changes

Assignees

@MartinWeindel MartinWeindel

@elankath elankath

@kon-angelo kon-angelo

Labels
area/auto-scaling Auto-scaling (CA/HPA/VPA/HVPA, predominantly control plane, but also otherwise) related kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/rebase Needs git rebase needs/review Needs review platform/aws Amazon web services platform/infrastructure size/m Size of pull request is medium (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.

Support extended resources in provider config node template without re-specifying core resources
8 participants
@elankath @gardener-robot @AndreasBurger @MartinWeindel @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3 @kon-angelo