Skip to content

Conversation

@dkokkino
Copy link

@dkokkino dkokkino commented Mar 4, 2025

The current implementation accesses the MetaData service exclusively over HTTP IPv4. If this attempt fails, a fatal error is thrown.

The new implementation follows the below logic:
- Config-drive
- IPv6 over HTTPS
- IPv6 over HTTP
- IPv4 over HTTPS
- IPv4 over HTTP
- Fatal error

Changes the logic to how the MetaData is reached. New implementation follows the logic below:
- Config-drive
- IPv6 over HTTPS
- IPv6 over HTTP
- IPv4 over HTTPS
- IPv4 over HTTP
- Report an error
@github-actions
Copy link

github-actions bot commented Mar 4, 2025

Thanks for your PR,
To run vendors CIs, Maintainers can use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs, Maintainers can use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

metaData, networkData, err = getOpenstackDataFromMetadataService()
if err != nil {
return metaData, networkData, fmt.Errorf("GetOpenStackData(): error getting OpenStack data: %w", err)
// Attempt to reach MetaData over IPv6 then over IPv4 for both HTTPS and HTTP
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe most of the deployments will be IPv4, so maybe we should try IPv4 first and if it fails, we try IPv6.

Copy link
Author

Choose a reason for hiding this comment

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

Okay i changed the logic to firstly check IPv4 then IPv6

// Attempt to reach MetaData over IPv6 then over IPv4 for both HTTPS and HTTP
reachedMetaData := false
for i, baseURL := range ospBaseURLS {
isIPv6 := i == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

One idea: maybe we could remove the usage of a loop given the only possible addresses are either IPv4 or IPv6 and that list won't grow.

Copy link
Author

Choose a reason for hiding this comment

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

I agree a loop is not necessary but I prefer using one because it allows us to exit as soon as we find the first address that can reach the metadata gracefully.

}

for _, intf := range interfaces {
if intf.Flags&network.FlagUp != 0 && intf.Flags&network.FlagLoopback == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should check if there is at least one IPv6 address defined in the interface to be used

Copy link
Author

Choose a reason for hiding this comment

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

Okay i added this check thanks for the feedback

- Changed the order to check IPv4 first
- Added a check to make sure there is at least one IPv6 address defined in the interface.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 13856261115

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 52 (0.0%) changed or added relevant lines in 1 file are covered.
  • 673 unchanged lines in 11 files lost coverage.
  • Overall coverage decreased (-0.6%) to 48.251%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/platforms/openstack/openstack.go 0 52 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/daemon/config.go 2 93.75%
pkg/host/manager.go 2 93.75%
controllers/sriovnetworknodepolicy_controller.go 3 58.66%
pkg/consts/constants.go 4 50.0%
controllers/generic_network_controller.go 5 74.38%
pkg/drain/drainer.go 19 65.77%
cmd/sriov-network-config-daemon/start.go 37 8.97%
cmd/sriov-network-config-daemon/service.go 46 75.73%
controllers/drain_controller_helper.go 47 64.84%
pkg/helper/mock/mock_helper.go 250 37.46%
Totals Coverage Status
Change from base Build 13625442222: -0.6%
Covered Lines: 7350
Relevant Lines: 15233

💛 - Coveralls

encodedSign := "%25"
port := "80"
if isIPv6 {
urls = append(urls, ospHTTPS+"["+baseURL+encodedSign+activeInterface+"]:"+port)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi question can you please explain why we need the interface name here?

base on this we can just send a request to the ipv6 address and it should be good no?

https://specs.openstack.org/openstack/neutron-specs/specs/ussuri/metadata-add-ipv6-support.html

Copy link
Author

@dkokkino dkokkino Mar 26, 2025

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I get it thanks!

@nvidia-ci-cd
Copy link

Can one of the admins verify this patch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants