Auto-detect the default BYOH interface for hosts #477
Conversation
|
@VibhorChinda, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction. |
|
@anusha94 @sachinkumarsingh092 have a look :) |
|
Thanks for working on this issue @VibhorChinda, I guess there is some code as well to handle the substitution of the default network interface name which could be removed. @anusha94 We could also look at whether to keep the TemplateParser in cloudinit package |
|
We also need to handle the logic for manual detection in |
by handling logic @sachinkumarsingh092 do you mean I should delete this function as now it will be automatic and no need for manual detection ?? |
ok @dharmjit will remove this function |
Yup, agreed. We should remove all code that is unused. Bonus - these didn't have unit tests either :) |
|
@VibhorChinda Please rebase from main - we just fixed the golangci-lint check that is failing |
|
I don't see any other utility of GetNetworkStatus() and UpdateNetwork() apart from updating the network status, which will be done by kube-vip anyway. So I think it should be safe to remove these. @anusha94 @dharmjit wdyt? |
|
@sachinkumarsingh092 I'm all for removing code as long as the pipeline remains green! |
|
I read all the comments and tried to carry out the changes required but i guess it would be not simple process because functions which have to be removed are being called some where in the codebase :|| for eg :-
|
|
@VibhorChinda if / when you remove a function, it is better to remove the references too instead of passing nil. |
VibhorChinda
force-pushed
the
autoDetect_BYOH_Host
branch
2 times, most recently
from
ad59020
to
927235d
Compare
|
@anusha94 @sachinkumarsingh092 @dharmjit is there any way to duplicate the tests locally ?? Like for CAPI stuff, I used to run commands "make verify-modules" & "make test" that used to duplicate the tests locally and I could make changes as required for making them pass :)) |
|
We also follow CAPI style make targets, albeit not that extensively yet. So if you see it in the Makefile, we have a target for |
Thanks will do from now on :)) |
|
Word of caution. At the moment, our tests (unit / integration / e2e) run only on Linux - more specifically Ubuntu 20.04 |
|
Hey everyone was working on this thing yesterday. But pretty much can't figure out why my tests are failing. Some help will be good :)) |
|
@VibhorChinda as you change the functionality, you should also change the corresponding tests. Yor CI fails as you haven't removed the test spec for the network status here after you removed the functionality, though I'm a bit skeptic about removing such test. But for now, just try this. |
@sachinkumarsingh092 what you said totally make sense. My CI failed because of this. But ig the e2e should have passed (It should create a workload cluster with single BYOH host what ever may be the change right ??) |
VibhorChinda
force-pushed
the
autoDetect_BYOH_Host
branch
from
927235d
to
ef48d7f
Compare
Codecov Report
@@ Coverage Diff @@
## main #477 +/- ##
==========================================
+ Coverage 67.11% 67.19% +0.07%
==========================================
Files 22 22
Lines 1721 1719 -2
==========================================
Hits 1155 1155
+ Misses 494 492 -2
Partials 72 72
|
|
All right! We're getting close. I can narrow down the e2e failures to something related to the control plane. Can you please go through the particular tests that are failing in |
|
Hey @sachinkumarsingh092 I was trying to debug this, but the problem is I am not able to replicate the tests locally in my environment :|| And here the I am not getting anything substantial with which we can move ahead, just we know the test which is failing but why is still a question :(( |
|
At last was successful to run tests locally :)))) Found the following error message : Failed to generate the manifest for "infrastructure-byoh" / "v0.1.0" Could we get anything from this ?? @sachinkumarsingh092 |
|
It seems that either you don't have |
|
Yes @sachinkumarsingh092 u were right kustomize was now installed on my setup. e2e-tests working fine now. All three tests are failing due to timeout error After running the "journalctl -xeu kubelet " in the "test-kpd8d6-control-plane" I found this in the logs : Apr 18 13:13:44 test-kpd8d6-control-plane kubelet[252]: E0418 13:13:44.941812 252 kubelet.go:2422] "Error getting node" err="node "test-kpd8d6-control-plane" not found" I can see that network plugin is not being installed on the control plane node. Which makes it unreachable and all tests goes in timeout error. What do you think ?? |
|
PS : Sorry for delay replies, it takes me sometime to get some headway. Still a beginner :)) |
|
@VibhorChinda a small tip: whenever giving a code block or some logs, put them inside a code block like below. It makes it easier to read and understand the logs :) The last line seems to suggest that you don't have any CNI plugins installed. The kindnet CNI plugins are added during the e2e test in the Makefile. So this should've worked. Can you confirm that the directory Alternatively, can you try installing a CNI and then confirm that it's working. That'd mean it's a problem while installing the CNI. You can use the following to install it: Ref:
Don't worry about it. Take your time. We're all learning here :) |
Makes sense, will take care of this from now on ✌️ |
|
The last line seems to suggest that you don't have any CNI plugins installed. The kindnet CNI plugins are added during the e2e test in the Makefile. So this should've worked. Can you confirm that the directory yep @sachinkumarsingh092 this thing exists |
|
Tried installing the CNI using this command "kubectl apply -f test/e2e/data/cni/kindnet/kindnet.yaml" on the control plane node. On the other hand, tried using the command on the whole project not specifically inside the control plane node. As I can see the same path in the code base. |
|
@VibhorChinda, VMware has approved your signed contributor license agreement. |
|
Sorry for the late reply @VibhorChinda. Got caught up in a few things.
Yes, that's because the control plane doesn't import the repository.
Can you share the logs? |
|
Hey @sachinkumarsingh092 sharing the logs, can take some time. My ubuntu workstation seems to have some issues. |
|
@VibhorChinda Any more update to this PR? |
|
Hey @anusha94 no updates as such. PS : Sorry for blocking the issue/PR for so long. |
What this PR does / why we need it:
Makes changes in the codebase to implement the auto-detect the default BYOH interface for hosts.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes # #466
Additional information
Changed the template files [deleted the vim_interface env variable]
Special notes for your reviewer