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

util/ipvs: include github.com/moby/ipvs #118031

Closed
wants to merge 1 commit into from
Closed

util/ipvs: include github.com/moby/ipvs #118031

wants to merge 1 commit into from

Conversation

uablrek
Copy link
Contributor

@uablrek uablrek commented May 16, 2023 •
edited
Loading

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

The github.com/moby/ipvs seems unmaintained and need fixes. This commit includes the code in the K8s tree. LICENSE is included as-is (must be fixed most likely). Unit-test are omitted since they require NET_ADMIN rights.

Most urgent is to fix moby/ipvs#27

But also to convert unstructured logging to structured (using logr probably)

Which issue(s) this PR fixes:

Special notes for your reviewer:

This is an attempt to solve a "unmaintained project" problem discussed in sig/network slack.

Does this PR introduce a user-facing change?

NONE

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

N/A

util/ipvs: include github.com/moby/ipvs
a299fbe 
The github.com/moby/ipvs seems unmaintained and need fixes.
This commit includes the code in the K8s tree. LICENSE is
included as-is (must be fixed most likely). Unit-test are omitted
since they require NET_ADMIN rights.
@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 16, 2023
@k8s-ci-robot
Copy link
Contributor

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-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label May 16, 2023
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-priority Indicates a PR lacks a `priority/foo` label and requires one. label May 16, 2023
@uablrek uablrek marked this pull request as draft May 16, 2023 06:42
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/dependency Issues or PRs related to dependency changes area/ipvs 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 May 16, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: uablrek
Once this PR has been reviewed and has the lgtm label, please assign thockin for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot requested review from caseydavenport, johnbelamaric and a team May 16, 2023 06:43
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 16, 2023
@pohly
Copy link
Contributor

pohly commented May 16, 2023

But also to moby/ipvs#27 (comment) (using logr probably)

See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md for logging. It's based on logr.

@danwinship
Copy link
Contributor

We should not have "external" code in pkg (and I suspect you won't be able to get pull-kubernetes-verify to pass the way this is). Either it should go into third_party/forked (as @aojea suggested on slack) or we should fully fork and "k8sify" it, merging it with the existing pkg/util/ipvs, porting to klog and other k8s coding conventions, removing any parts of it we'll never use, etc.

@uablrek
Copy link
Contributor Author

uablrek commented May 27, 2023

or we should fully fork and "k8sify" it, merging it with the existing pkg/util/ipvs, porting to klog and other k8s coding conventions, removing any parts of it we'll never use, etc.

I will probably go for this option, but in another PR

@uablrek uablrek closed this May 27, 2023
@uablrek uablrek mentioned this pull request May 27, 2023
Merged
@uablrek uablrek mentioned this pull request Oct 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caseydavenport caseydavenport Awaiting requested review from caseydavenport

@johnbelamaric johnbelamaric Awaiting requested review from johnbelamaric

Assignees
No one assigned
Labels
area/dependency Issues or PRs related to dependency changes area/ipvs cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/network Categorizes an issue or PR as relevant to SIG Network. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Using this package without ipvs doesn't return error
4 participants
@uablrek @k8s-ci-robot @pohly @danwinship