-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
Adapt testpmd-lb-operator to operator-sdk v1.33.0 #53
Conversation
ramperher
commented
Feb 7, 2024
•
edited
Loading
edited
- Created new testpmd-lb-operator folder and creating operator-sdk v1.33.0 skeleton with the following command:
- Rest of the work was mainly to move most of loadbalancer role to the project, check all files created, and adapt Makefile to the new structure
6c6c26c
to
045a332
Compare
Okey, so the issue with the operator build process is really because of the new Makefile format, I'll be adding the new features step by step to detect what was wrong. |
d1e5b61
to
08fcd84
Compare
I've managed to correctly build the operator, the issue was with some new comments labelled in this way: |
The job I've launched will fail in one of the tasks from new sriov config role included in the collections, that needs to be fixed before, cc @tonyskapunk |
from change #53: |
Current issue is with LoadBalancer creation:
I need to recheck the Role we're creating here, it's not allowing network-attachment-definitions by default and we should add it, in the same way we were doing before. |
from change #53: |
from change #53: |
Change looks good, I've tested the whole chain with tnf and preflight, example-cnf test looks good too. This is ready for review, @tkrishtop , could you have some time to take a look to confirm the structure is fine? There are a lot of files that have been modified but I've just put what was generated by operator-sdk v1.33.0 |
Nice work Ramon, it would be great to have these steps somewhere in the doc, if they are not already : ) |
Sure, I can add the steps I have followed to the README, then I will rebase because I have merged the change for the service accounts. I will do this on Monday :) |
4966977
to
32dbdb8
Compare
@manurodriguez , I've updated the README and I've rebased the change, hope it's fine now. |
32dbdb8
to
c20611f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ramperher I have only checked a part of the PR (it's huge), so only submitting partial review for the moment, sorry for that.
@tkrishtop , just FYI, most of the changes are formatting changes related to the new structure generated by operator-sdk v1.33.0, compared to v1.10.0 I've also addressed all your comments, please take a look when you have some time. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ramperher thank you for the great work!
I carefully validated the logs of the job running grep on the log archive $ grep -a testpmd-lb *
, I see testpmd-lb pod running fine and there are no errors related to the deployment
Created container manager
events.txt:example-cnf 14m Normal Started pod/testpmd-lb-operator-controller-manager-7bdd56f9b6-m9xk5 Started container manager
events.txt:example-cnf 14m Normal LeaderElection lease/testpmd-lb-operator testpmd-lb-operator-controller-manager-7bdd56f9b6-m9xk5_4fdc01b1-c1df-4815-84ff-7e37a7dc0310 became leader
example-cnf_events.log:14m Normal SuccessfulCreate replicaset/testpmd-lb-operator-controller-manager-7bdd56f9b6 Created pod: testpmd-lb-operator-controller-manager-7bdd56f9b6-m9xk5
Normal InstallSucceeded clusterserviceversion/testpmd-lb-operator.v0.2.13-pr53.49669774 install strategy completed with no errors
Thank you @manurodriguez and @tkrishtop for the review, I'll merge this and rebase the change for cnf-app-mac-operator, thanks! |