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.

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

Mock and unit testcases added for windows kube-proxy. #111031

Conversation

princepereira
Copy link
Contributor

@princepereira princepereira commented Jul 8, 2022

What type of PR is this?

/kind cleanup
/kind failing-test
/kind windows
/kind kube-proxy

What this PR does / why we need it:

This is to add mock and unit tests for windows kube-proxy

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot
Copy link
Contributor

@princepereira: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. labels Jul 8, 2022
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 8, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: princepereira / name: PRINCE PEREIRA (3663842fec0fe26d3b6eabcff060aff429bd4def)

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 8, 2022
@k8s-ci-robot
Copy link
Contributor

Welcome @princepereira!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @princepereira. Thanks for your PR.

I'm waiting for a kubernetes 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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/network Categorizes an issue or PR as relevant to SIG Network. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 8, 2022
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 8, 2022
@princepereira princepereira force-pushed the ppereira-windows-kubeproxy-ut branch 2 times, most recently from 7685d57 to 07dbeb5 Compare July 13, 2022 10:32
@k8s-ci-robot k8s-ci-robot 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 Jul 13, 2022
@jsturtevant
Copy link
Contributor

/sig windows
/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/windows Categorizes an issue or PR as relevant to SIG Windows. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 18, 2022
@jsturtevant
Copy link
Contributor

alternative/related to #110749

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2022
)

// mockHCN saves the created endpoints and loadbalancers in slices to be able to work with them easier
type mockHCN struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to mock all the hcnUtils? To my understanding all of these are helpers that ultimately call into HCN.

Would it perhaps be better to only mock all of the hcsshim/HCN signature that the hcnUtils use? Store any of the created endpoints/loadbalancers as part of the "HCN" mock in memory and remove the structs/interfaces that mock hcnUtils. Tests and proxier code would use the same hcnUtils code.

hcnUtils for tests would initialize the hcn mock package which store artificial endpoints/loadbalancers, and real proxier code would use hcnUtils that initializes/references real hcn package.

Maybe I am missing something but feels to me this might reduce a layer of abstraction and make the code easier to understand, without reducing robustness of tests. Curious to hear your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hcnUtils is the final place where the test cases have control to mock the function call in the existing implementation. Considering an example of network.createEndpoint():

network.createEndpoint() -Calls-> createEndpoint [Which is in hcn package of vendor directory] -Calls-> hcnOpenNetwork [Which is in hcn package of vendor directory] -Calls-> syscall.Syscall

If you have a look at the above call flow, test code dont have any handle to avoid the syscall. That's the reason behind mocking hncUtils.

And sorry for the delay in responding, missed out the comment somehow.

@princepereira
Copy link
Contributor Author

@daschott , Please review the code when you get a chance.

@marosset marosset removed this from In Progress (v1.25) in SIG-Windows Sep 21, 2022
@k8s-ci-robot
Copy link
Contributor

@princepereira: PR needs rebase.

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2022
@marosset
Copy link
Contributor

/milestone v1.27
/triage accepted
/priority important-soon

@k8s-ci-robot k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Nov 17, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.27 milestone Nov 17, 2022
@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 17, 2022
@dims
Copy link
Member

dims commented Dec 12, 2022

If you still need this PR then please rebase, if not, please close the PR

@dims
Copy link
Member

dims commented Dec 12, 2022

If you still need this PR then please remove the merge commits (see bot comment above), if not, please close the PR

@claudiubelu
Copy link
Contributor

/cc @claudiubelu

@claudiubelu
Copy link
Contributor

/assign

@furkatgofurov7
Copy link
Member

@princepereira @claudiubelu HI! Release Bug Triage Shadow here.

This PR hasn't been updated in 3 months. I just wanted to check if this is still on track for 1.27 release?

@furkatgofurov7
Copy link
Member

Hi @princepereira!

K8s 1.27 Bug Triage Shadow here again.
FYI, with Code Freeze approaching next week on March 15th, if you would like this PR to be part of the v1.27 release, please make sure it goes in before above mentioned date. Post Code Freeze, we will only be able to accept Release Blockers and Critical issues. Thank you!

@dims
Copy link
Member

dims commented Mar 10, 2023

@princepereira please rebase!

@liggitt
Copy link
Member

liggitt commented Mar 21, 2023

/milestone clear

since this is touching code outside of test files after code freeze

@k8s-ci-robot k8s-ci-robot removed this from the v1.27 milestone Mar 21, 2023
@dims
Copy link
Member

dims commented May 3, 2023

Please remove merge commits, rebase and reopen this PR if necessary. It will not be merged as-is

@dims dims closed this May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Archived in project
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

9 participants