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

chore: add DOCA driver matrix validation #1251

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

chore: add DOCA driver matrix validation #1251

wants to merge 1 commit into from
+147 −10

Conversation

rollandf
Copy link
Member

No description provided.

@coveralls
Copy link
Collaborator

coveralls commented Jan 13, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 12851631605

Details

  • 0 of 83 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.4%) to 59.774%
Changes Missing Coverage Covered Lines Changed/Added Lines %
hack/release.go 0 83 0.0%
Totals Coverage Status
Change from base Build 12830426562: -0.4%
Covered Lines: 3339
Relevant Lines: 5586
💛 - Coveralls

@rollandf rollandf marked this pull request as draft January 13, 2025 16:01
@rollandf rollandf force-pushed the verify-doca-driver branch 2 times, most recently from 06d07d9 to cde80a0 Compare January 13, 2025 16:34
@rollandf
Copy link
Member Author 

$ make check-doca-drivers

wget https://raw.githubusercontent.com/Mellanox/doca-driver-build/refs/heads/main/release_manifests/25.01-TBD.yaml  -O /home/frolland/git/network-operator/hack/tmp/doca-driver-matrix.yaml
--2025-01-13 20:41:16--  https://raw.githubusercontent.com/Mellanox/doca-driver-build/refs/heads/main/release_manifests/25.01-TBD.yaml
Resolving raw.githubusercontent.com (raw.githubusercontent.com)... 185.199.109.133, 185.199.110.133, 185.199.111.133, ...
Connecting to raw.githubusercontent.com (raw.githubusercontent.com)|185.199.109.133|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: 627 [text/plain]
Saving to: ‘/home/frolland/git/network-operator/hack/tmp/doca-driver-matrix.yaml’

/home/frolland/git/network-operato 100%[================================================================>]     627  --.-KB/s    in 0s      

2025-01-13 20:41:17 (28.8 MB/s) - ‘/home/frolland/git/network-operator/hack/tmp/doca-driver-matrix.yaml’ saved [627/627]

cd hack && go run release.go --doca-driver-check --doca-driver-matrix /home/frolland/git/network-operator/hack/tmp/doca-driver-matrix.yaml
Error: missing os-arch combination: ubuntu24.04-arm64
exit status 1
make: *** [Makefile:427: check-doca-drivers] Error 1

@rollandf rollandf marked this pull request as ready for review January 14, 2025 08:12
heyvister1
heyvister1 reviewed Jan 14, 2025
View reviewed changes
hack/release.go
break
}
}
if !found {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit maybe you should use unfound as a list to indicate all missing images. Otherwise will need to trigger this job to discover all missing ones

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

heyvister1
heyvister1 reviewed Jan 14, 2025
View reviewed changes
hack/release.go
func validateTags(config DocaDriverMatrix, tags []string, version string) error {
// Build expected OS-arch combinations
expectedCombinations := make(map[string]struct{})
for _, entry := range config.DynamicallyCompiled {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did we miss precompiled list here? I don't see code is verifying it

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR only checks 'DynamicallyCompiled'.
For precompiled, it will need additional work as the info for kernels are not available in the matrix until GA.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok. Did you open a ticket for tag validation task? If so then we'll need to update that its still pending precompiled verification

Copy link
Member Author

Choose a reason for hiding this comment

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

The intent here is to have a minimal check that we have the required 'dynamically_compiled'.
Any more complex use case can be added later, maybe as a Jenkins job

heyvister1
heyvister1 reviewed Jan 14, 2025
View reviewed changes
Makefile Outdated
.PHONY: check-doca-drivers
check-doca-drivers: $(HACKTMPDIR)
$(eval DOCAVERSION := $(shell yq '.Mofed.version' hack/release.yaml | cut -d'-' -f1))
wget $(DOCA_DRIVER_RELEASE_URL)$(DOCAVERSION)-TBD.yaml -O $(HACKTMPDIR)/doca-driver-matrix.yaml
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are not using DOCAVERSION (e.g. 2.10) here rather than ofed driver version (e.g. 25.01-0.2.7.0-0)

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed var name

rollandf
rollandf commented Jan 14, 2025
View reviewed changes
Makefile Outdated
.PHONY: check-doca-drivers
check-doca-drivers: $(HACKTMPDIR)
$(eval DRIVERVERSION := $(shell yq '.Mofed.version' hack/release.yaml | cut -d'-' -f1))
wget $(DOCA_DRIVER_RELEASE_URL)$(DRIVERVERSION)-TBD.yaml -O $(HACKTMPDIR)/doca-driver-matrix.yaml
Copy link
Member Author

Choose a reason for hiding this comment

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

@heyvister1 Regarding the file name in doca-driver-build repo, maybe we should have only the 25.01 in it and add a field in the file with the GA version.
WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm totally ok with that. Please also add an option to use OFED version externally, in case its still TBD (non-GAed) so we can run this verification locally.

Copy link
Member Author

Choose a reason for hiding this comment

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

So now, it takes the version specified in the release.yaml. I think it is good enough for now. We can extend later if needed.

@heyvister1
Copy link
Collaborator

We also need to make sure this verification walks though LTS versions as well, according to manifest folder

@rollandf rollandf mentioned this pull request Jan 14, 2025
Merged
@rollandf rollandf requested a review from e0ne January 15, 2025 09:51
@rollandf
Copy link
Member Author

/retest-blackduck_scan

@rollandf
Copy link
Member Author

/retest-nic_operator_helm

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

@heyvister1 heyvister1 heyvister1 left review comments

@e0ne e0ne Awaiting requested review from e0ne

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rollandf @coveralls @heyvister1