Skip to content

Conversation

@4ch3los
Copy link
Contributor

@4ch3los 4ch3los commented Jun 25, 2025

What this PR does / why we need it:
This PR completes the feature of multiple nics for vmware cloud director workers(introduced in kubermatic/machine-controller#1941 & kubermatic/operating-system-manager#471 ). By Allowing to define additional networks, which will also be added to the machinedeployment

Which issue(s) this PR fixes:

What type of PR is this?
/kind feature

Special notes for your reviewer:
This PR depends on kubermatic/machine-controller#1941 & kubermatic/operating-system-manager#471

Does this PR introduce a user-facing change? Then add your Release Note here:

You can now define multiple Networks for nodes of your vmware cloud director machinedeployment

Documentation:

NONE

@kubermatic-bot kubermatic-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. docs/none Denotes a PR that doesn't need documentation (changes). dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. sig/api Denotes a PR or issue as being assigned to SIG API. labels Jun 25, 2025
@kubermatic-bot
Copy link
Contributor

Hi @4ch3los. Thanks for your PR.

I'm waiting for a kubermatic member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@kubermatic-bot kubermatic-bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 25, 2025
@csengerszabo csengerszabo requested a review from kron4eg July 2, 2025 09:10
Copy link
Contributor

@KhizerRehan KhizerRehan left a comment

Choose a reason for hiding this comment

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

Minor Feedback (PTAL)

@Waseem826
Copy link
Contributor

/ok-to-test

@kubermatic-bot kubermatic-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 22, 2025
4ch3los and others added 3 commits August 26, 2025 10:36
…ctor/template.html

Co-authored-by: Khizer Rehan <[email protected]>
Signed-off-by: Kai Fink <[email protected]>
…ctor/component.ts

Co-authored-by: Khizer Rehan <[email protected]>
Signed-off-by: Kai Fink <[email protected]>
Signed-off-by: Kai Fink <[email protected]>
@KhizerRehan
Copy link
Contributor

@4ch3los can you run make update-codegen inside the api folder?

@4ch3los
Copy link
Contributor Author

4ch3los commented Sep 25, 2025

@4ch3los can you run make update-codegen inside the api folder?

@KhizerRehan
Yeah sure, but i guess we wont get the build + tests running until the machine-controller changes are released(kubermatic/machine-controller#1941) and the dependency can be updated

@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign khizerrehan for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@kubermatic-bot kubermatic-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 4, 2025
@4ch3los
Copy link
Contributor Author

4ch3los commented Nov 4, 2025

Hey There! OSM and Machinecontroller changes have been released and i updated the dependencies, client, codegen etc. the only thing failing is the unit tests, but im not exactly sure why, does anybody of you have a hint? @KhizerRehan @Waseem826
Thanks!

Copy link
Contributor

@KhizerRehan KhizerRehan left a comment

Choose a reason for hiding this comment

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

Minor Feedback


private _initForm(): void {
const values = this._nodeDataService.nodeData.spec.cloud.vmwareclouddirector;
const defaults = getDefaultNodeProviderSpec(NodeProvider.VMWARECLOUDDIRECTOR) as VMwareCloudDirectorNodeSpec;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add default value for additional network in getDefaultNodeProviderSpec method under case NodeProvider.VMWARECLOUDDIRECTOR:

additionalNetworks: []

@KhizerRehan
Copy link
Contributor

/test pre-dashboard-api-unit

@KhizerRehan KhizerRehan requested a review from Copilot November 5, 2025 15:03
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for configuring multiple network interfaces for VMware Cloud Director worker nodes in the UI, complementing backend support added in related machine-controller and operating-system-manager PRs. Users can now specify additional networks beyond the primary network when creating machine deployments.

Key Changes:

  • Added additionalNetworks field to VMware Cloud Director node specifications across frontend and backend
  • Implemented UI controls for selecting multiple networks with validation to prevent duplicate selection
  • Updated network configuration logic to handle both primary and additional networks as a unified list

Reviewed Changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
modules/web/src/app/shared/entity/node.ts Added additionalNetworks array field to VMwareCloudDirectorNodeSpec
modules/web/src/app/node-data/basic/provider/vmware-cloud-director/template.html Added multi-select dropdown for additional networks with conditional display
modules/web/src/app/node-data/basic/provider/vmware-cloud-director/component.ts Implemented logic for managing additional networks selection and synchronization
modules/api/pkg/resources/machine/common.go Updated provider config generation to merge primary and additional networks into unified list
modules/api/pkg/machine/convert.go Added conversion logic between machine spec networks and API node spec format
modules/api/pkg/api/v1/types.go Added AdditionalNetworks field to VMwareCloudDirectorNodeSpec type
modules/api/pkg/test/e2e/utils/apiclient/models/*.go Regenerated OpenAPI client models for new fields and Kubernetes API updates
modules/api/go.mod Updated dependencies including k8s.io, controller-runtime, and related libraries
modules/api/cmd/kubermatic-api/swagger.json Updated swagger specification with new field definitions
modules/api/pkg/test/e2e/utils/kubernetes.go Updated RESTClientForGVK call signature for new API version

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

const networkControl = this.form.get(Controls.Network);
const additionalNetworksControl = this.form.get(Controls.AdditionalNetworks);

additionalNetworksControl.setValue(additionalNetworksControl.value.filter(n => this.networks.includes(n)));
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

Potential nil reference if additionalNetworksControl.value is null or undefined. Add a null check before calling filter: additionalNetworksControl.value?.filter(n => this.networks.includes(n)) ?? []

Suggested change
additionalNetworksControl.setValue(additionalNetworksControl.value.filter(n => this.networks.includes(n)));
additionalNetworksControl.setValue(additionalNetworksControl.value?.filter(n => this.networks.includes(n)) ?? []);

Copilot uses AI. Check for mistakes.
@KhizerRehan
Copy link
Contributor

KhizerRehan commented Nov 6, 2025

Hi @4ch3los, based on the logs it doesn’t seem like this PR caused the pipeline failure. However, something has changed that’s affecting this line, as all failures point to it.

Expected VS Got

Example

image

@KhizerRehan
Copy link
Contributor

KhizerRehan commented Nov 7, 2025

FYI: @4ch3los

The following test case failures are due to dependency updates the preference: {} property was removed during the migration to Kubernetes v1.34. However, there’s one PR that needs to be merged first. Once that’s done, rebasing onto the latest main branch should resolve the pipeline failures.

Ref: #7692

@KhizerRehan
Copy link
Contributor

@4ch3los can you rebase your PR with latest main branch?

# Conflicts:
#	modules/api/go.mod
#	modules/api/go.sum
@kubermatic-bot kubermatic-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 18, 2025
@kubermatic-bot
Copy link
Contributor

@4ch3los: 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
pre-dashboard-web-integration-tests-ce 69c2b8e link true /test pre-dashboard-web-integration-tests-ce
pre-dashboard-web-integration-tests 69c2b8e link true /test pre-dashboard-web-integration-tests
pre-dashboard-api-e2e 69c2b8e link true /test pre-dashboard-api-e2e

Full PR test history

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.

@4ch3los
Copy link
Contributor Author

4ch3los commented Nov 18, 2025

@4ch3los can you rebase your PR with latest main branch?

Did it, and seems that the pipeline failed cause of some temporary conditions again

@kron4eg kron4eg removed their request for review November 18, 2025 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. docs/none Denotes a PR that doesn't need documentation (changes). kind/feature Categorizes issue or PR as related to a new feature. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api Denotes a PR or issue as being assigned to SIG API. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants