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

Switch to k8s.io/utils/inotify #80689

Merged
merged 3 commits into from Aug 8, 2019
Merged

Conversation

Pothulapati
Copy link
Contributor

@Pothulapati Pothulapati commented Jul 28, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Replaces github.com/go-sigma/inotify with k8s.io/utils/inotify

Which issue(s) this PR fixes:

Fixes #80092

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.:


@dims

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/M Denotes a PR that changes 30-99 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. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 28, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @Pothulapati. 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 the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 28, 2019
@k8s-ci-robot k8s-ci-robot requested review from adohe-zz, andrewsykim and a team July 28, 2019 14:07
@k8s-ci-robot k8s-ci-robot added area/apiserver area/cloudprovider area/dependency Issues or PRs related to dependency changes area/kubectl area/kubelet sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 28, 2019
@Pothulapati Pothulapati changed the title Inotify Switch to k8s.io/utils/inotify Jul 28, 2019
@cblecker
Copy link
Member

Dupe of #80613
/close

@k8s-ci-robot
Copy link
Contributor

@cblecker: Closed this PR.

In response to this:

Dupe of #80613
/close

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.

@cblecker
Copy link
Member

cblecker commented Jul 28, 2019

Actually, I just saw that this was intended.
/reopen

@k8s-ci-robot k8s-ci-robot reopened this Jul 28, 2019
@k8s-ci-robot
Copy link
Contributor

@cblecker: Reopened this PR.

In response to this:

Actually, I just saw that this was intended.
/reopen
/ok-to-test

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.

@cblecker
Copy link
Member

/ok-to-test

@dims
Copy link
Member

dims commented Aug 6, 2019

/priority important-soon
/milestone v1.16

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 6, 2019
@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-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 6, 2019
@liggitt
Copy link
Member

liggitt commented Aug 6, 2019

any tests in k8s.io/utils or in k8s to prove kubernetes/utils#101 fixes the leak?

was this answered?

@dims
Copy link
Member

dims commented Aug 6, 2019

@liggitt hmm i thought i figured out, but missed adding a comment. Please see manual steps in the fsnotify PR which is very similar to this change

dims@bigbox:~/junk$ /bin/cat test.go
package main

import (
    "fmt"
    "log"
    "os"
    "os/exec"
    "time"

    "k8s.io/utils/inotify"
)

func main() {
    if len(os.Args) == 1 { // parent
        watcher, err := inotify.NewWatcher()
        if err != nil {
            log.Fatal(err)
        }
        defer watcher.Close()

        child := exec.Command(os.Args[0], "child")
        err = child.Start()
        if err != nil {
            log.Fatal(err)
        }

        fmt.Println("Parent", os.Getpid(), "child", child.Process.Pid)
    }

    fmt.Println(os.Args, "going into sleep")
    for {
        time.Sleep(time.Second)
    }
}
dims@bigbox:~/junk$ go run test.go
Parent 18870 child 18875
[/tmp/go-build617179765/b001/exe/test] going into sleep
dims@bigbox:~$ lsof -p 18870
COMMAND   PID USER   FD      TYPE DEVICE SIZE/OFF     NODE NAME
test    18870 dims  cwd       DIR  253,0     4096 41816258 /home/dims/junk
test    18870 dims  rtd       DIR  253,0     4096        2 /
test    18870 dims  txt       REG  253,0  1664448  1576577 /tmp/go-build617179765/b001/exe/test
test    18870 dims    0u      CHR  136,0      0t0        3 /dev/pts/0
test    18870 dims    1u      CHR  136,0      0t0        3 /dev/pts/0
test    18870 dims    2u      CHR  136,0      0t0        3 /dev/pts/0
test    18870 dims    3r  a_inode   0,13        0    14824 inotify
test    18870 dims    5u  a_inode   0,13        0    14824 [eventpoll]
dims@bigbox:~$ lsof -p 18875
COMMAND   PID USER   FD   TYPE DEVICE SIZE/OFF     NODE NAME
test    18875 dims  cwd    DIR  253,0     4096 41816258 /home/dims/junk
test    18875 dims  rtd    DIR  253,0     4096        2 /
test    18875 dims  txt    REG  253,0  1664448  1576577 /tmp/go-build617179765/b001/exe/test
test    18875 dims    0r   CHR    1,3      0t0        6 /dev/null
test    18875 dims    1w   CHR    1,3      0t0        6 /dev/null
test    18875 dims    2w   CHR    1,3      0t0        6 /dev/null

@liggitt
Copy link
Member

liggitt commented Aug 6, 2019

ok, glad there's a manual test. any possibility of an automated one? I won't block on it, but would like to at least see an issue for tracking that

@dims
Copy link
Member

dims commented Aug 6, 2019

@liggitt Added an issue - kubernetes/utils#106

@liggitt
Copy link
Member

liggitt commented Aug 6, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 6, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, Pothulapati

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

The pull request process is described 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 added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 6, 2019
@Pothulapati
Copy link
Contributor Author

Pothulapati commented Aug 7, 2019

Looks like #78610 , updated to use a more latest commig of k8s.io/utils and there were conflicts as this PR had an older commit.

I updated the PR, to use the latest commit of k8s.io/utils i.e same as the one in the PR. :)

@liggitt
Copy link
Member

liggitt commented Aug 7, 2019

sorry for the churn. does this need to be rebased on master? It looks like this still has commits that conflict with #78610. If so, I'd recommend:

  • rebasing on master, dropping the commits that mess with vendor
  • run hack/update-vendor.sh and make a new commit (you shouldn't need to do any pin-dependency work any more)
  • run hack/lint-dependencies.sh and make sure the results are clean

Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com>
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 7, 2019
@Pothulapati
Copy link
Contributor Author

Got it @liggitt, Thanks for the help!
Updated as per your comment :)

@liggitt
Copy link
Member

liggitt commented Aug 7, 2019

thanks

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2019
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 6d49d69 into kubernetes:master Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/cloudprovider area/code-organization Issues or PRs related to kubernetes code organization area/dependency Issues or PRs related to dependency changes area/kubectl area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch to k8s.io/utils/inotify
8 participants