[kwok] Provided service for logs #417

nikola-jokic · 2023-03-24T18:54:10Z

What type of PR is this?

/kind feature

Which issue(s) this PR fixes:

Fixes #217

Does this PR introduce a user-facing change?

Add logs logic with Logs and ClusterLogs type

netlify · 2023-03-24T18:54:16Z

✅ Deploy Preview for k8s-kwok canceled.

Name	Link
🔨 Latest commit	`60c1bc8`
🔍 Latest deploy log	https://app.netlify.com/sites/k8s-kwok/deploys/6432d62ea7e01700084d3d43

k8s-ci-robot · 2023-03-24T18:54:20Z

Hi @nikola-jokic. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

pkg/kwok/server/debugging_logs.go

test/kwokctl/kwokctl_logs_test.sh

pkg/kwok/server/debugging_logs.go

wzshiming · 2023-03-27T03:18:21Z

/ok-to-test

test/kwokctl/kwokctl_kind_logs.test.sh

nikola-jokic · 2023-03-31T09:44:52Z

/retest

pkg/kwokctl/runtime/kind/cluster.go

pkg/kwokctl/runtime/kind/kind.yaml.tpl

nikola-jokic · 2023-04-03T20:49:44Z

I am not sure why Mac does not allow /tmp/kwok to be mounted as a volume
This workflow run shows that /tmp/kwok exists, but the docker on Mac does not allow it
Ideally, I'd go with this by creating a file in /tmp, but then the kind would fail mounting tmp twice to the kind runtime.
I did not try ignoring /tmp mount in kind since it seems to me that we would tie implementation too much with the runtime, but that might be a way to go.
@wzshiming what do you think?

wzshiming

I think it's a permissions issue, we don't have access to /tmp/ on the Mac in CI

test/kwokctl/kwokctl_logs_test.sh

test/kwokctl/logs.yaml

nikola-jokic · 2023-04-04T18:46:16Z

Trying tests again

If they fail, I'll investigate it to see if there is a problem on the runner side... I executed logs tests on Mac, and they worked... It might be something similar to the hosted machine from the last time 😕

Co-authored-by: Shiming Zhang <wzshiming@foxmail.com>

debugging

Co-authored-by: Shiming Zhang <wzshiming@foxmail.com>

… 0o, use path instead of filepath

original

wzshiming

I've tried kubectl logs which works fine, kubectl logs -f which doesn't seem to work

wzshiming · 2023-04-09T09:41:08Z

pkg/kwok/server/debugging_logs.go

+func readLogs(ctx context.Context, logInfo *internalversion.Log, opts *logOptions, stdout, stderr io.Writer) error {
+	logger := log.FromContext(ctx)
+
+	logsFilePath := path.Join("/var/components/controller", logInfo.LogsFile)


I think this part of the logic should be handled in kwokctl, not in the kwok server.

Working on it
Thank you for reviews a lot! I wanted to push this to ask for an opinion

I'll have it ready now that I know everything how to structure it, and I'll test all scenarios
Thank you again!

nikola-jokic · 2023-04-09T19:30:06Z

pkg/kwokctl/runtime/util.go

+	volumes := make([]internalversion.Volume, 0, len(mountDirs))
+	i := 0
+	for dir := range mountDirs {
+		dirPath := strings.TrimPrefix(dir, "/var/components/controller")


This is the part I am unsure of. We can probably add strip prefix only in case when runtime is kind because we know that the prefix is going to be added, and it is going to be used in template.

The issue is that to use extra volume mounts, we add /var/components/controller to all paths.
Since we need correct configuration for logs, we expand the path. I agree that the server should not inspect the runtime from configuration, so that is why this change was necessary. Ideally it should not interfere with the binary runtime unless the actual path there is this constant. Because of that, It might be wise to use check for the runtime but I wanted to check first, since this approach might be flawed in a way I don't know 😄

nikola-jokic · 2023-04-09T19:30:56Z

Thank you for pointing out the follow. I included tests for tail, since and follow in e2e tests 😄

wzshiming · 2023-04-12T02:12:56Z

/approve
/lgtm
/label tide/merge-method-squash
/hold

I've tested it locally and it's all working as expected, great!
Unhold as you wish 😄

Also, add the same logic to Attach, do you think it's appropriate?

k8s-ci-robot · 2023-04-12T02:13:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nikola-jokic, wzshiming

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:

~~OWNERS~~ [wzshiming]

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

nikola-jokic · 2023-04-12T07:42:23Z

/unhold

Oh sure! Taking that one 😄

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. labels Mar 24, 2023

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 24, 2023

k8s-ci-robot requested review from wzshiming and yibozhuang March 24, 2023 18:54

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 Mar 24, 2023

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 24, 2023

nikola-jokic commented Mar 24, 2023

View reviewed changes

pkg/kwok/server/debugging_logs.go Show resolved Hide resolved

nikola-jokic commented Mar 24, 2023

View reviewed changes

test/kwokctl/kwokctl_logs_test.sh Outdated Show resolved Hide resolved

nikola-jokic changed the title ~~Nikola jokic/logs service~~ [kwok] Provided service for logs Mar 24, 2023

wzshiming reviewed Mar 27, 2023

View reviewed changes

pkg/kwok/server/debugging_logs.go Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 27, 2023

nikola-jokic force-pushed the nikola-jokic/logs-service branch from 4ff104a to 533d3ce Compare March 29, 2023 18:17

nikola-jokic commented Mar 29, 2023

View reviewed changes

test/kwokctl/kwokctl_kind_logs.test.sh Show resolved Hide resolved

nikola-jokic force-pushed the nikola-jokic/logs-service branch from 5f18fd8 to b7607fb Compare March 31, 2023 08:08

nikola-jokic marked this pull request as ready for review March 31, 2023 09:47

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 31, 2023

k8s-ci-robot requested a review from wzshiming March 31, 2023 09:47

wzshiming reviewed Mar 31, 2023

View reviewed changes

pkg/kwokctl/runtime/kind/cluster.go Outdated Show resolved Hide resolved

pkg/kwokctl/runtime/kind/kind.yaml.tpl Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2023

nikola-jokic force-pushed the nikola-jokic/logs-service branch from e045c22 to 8baa9b5 Compare April 3, 2023 07:46

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2023

wzshiming reviewed Apr 4, 2023

View reviewed changes

test/kwokctl/kwokctl_logs_test.sh Outdated Show resolved Hide resolved

test/kwokctl/logs.yaml Outdated Show resolved Hide resolved

nikola-jokic and others added 8 commits April 8, 2023 19:00

Update test/kwokctl/kwokctl_logs_test.sh

29959fa

Co-authored-by: Shiming Zhang <wzshiming@foxmail.com>

Support relative paths in logs, removed accidential snapshot from

83c18e8

debugging

Apply suggestions from code review

7b62ed0

Co-authored-by: Shiming Zhang <wzshiming@foxmail.com>

Apply suggestions from code review

08818c0

Co-authored-by: Shiming Zhang <wzshiming@foxmail.com>

Merge extra volumes in KwokController, remove gofmt boring updates on…

0f7ca92

… 0o, use path instead of filepath

Fix linting errors

1a17e17

fix linter issue and typo

98383fe

Fix volume mounts adding fallback from /var/components/controller to

3cb49b8

original

nikola-jokic force-pushed the nikola-jokic/logs-service branch from 8e157d9 to 3cb49b8 Compare April 8, 2023 20:23

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 8, 2023

nikola-jokic added 2 commits April 8, 2023 22:30

gofumpt 0o...

f480a5a

remove rebase error I made

92accb0

wzshiming reviewed Apr 9, 2023

View reviewed changes

nikola-jokic added 2 commits April 9, 2023 15:20

Move mounting logic to kwokctl, add tests for tail, since and follow

929bfa9

increase sleep for follow logs to 5s

60c1bc8

nikola-jokic commented Apr 9, 2023

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. labels Apr 12, 2023

k8s-ci-robot assigned wzshiming Apr 12, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 12, 2023

wzshiming mentioned this pull request Apr 12, 2023

[kwok] Simulate the services provided by Kubelet #162

Closed

5 tasks

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 12, 2023

k8s-ci-robot merged commit 7e44951 into kubernetes-sigs:main Apr 12, 2023

nikola-jokic deleted the nikola-jokic/logs-service branch April 12, 2023 08:10

wzshiming added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Apr 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[kwok] Provided service for logs #417

[kwok] Provided service for logs #417

nikola-jokic commented Mar 24, 2023 •

edited by wzshiming

Loading

netlify bot commented Mar 24, 2023 •

edited

Loading

k8s-ci-robot commented Mar 24, 2023

wzshiming commented Mar 27, 2023

nikola-jokic commented Mar 31, 2023

nikola-jokic commented Apr 3, 2023

wzshiming left a comment

nikola-jokic commented Apr 4, 2023

wzshiming left a comment

wzshiming Apr 9, 2023

nikola-jokic Apr 9, 2023

nikola-jokic Apr 9, 2023

nikola-jokic commented Apr 9, 2023

wzshiming commented Apr 12, 2023

k8s-ci-robot commented Apr 12, 2023

nikola-jokic commented Apr 12, 2023

[kwok] Provided service for logs #417

[kwok] Provided service for logs #417

Conversation

nikola-jokic commented Mar 24, 2023 • edited by wzshiming Loading

What type of PR is this?

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

netlify bot commented Mar 24, 2023 • edited Loading

✅ Deploy Preview for k8s-kwok canceled.

k8s-ci-robot commented Mar 24, 2023

wzshiming commented Mar 27, 2023

nikola-jokic commented Mar 31, 2023

nikola-jokic commented Apr 3, 2023

wzshiming left a comment

Choose a reason for hiding this comment

nikola-jokic commented Apr 4, 2023

wzshiming left a comment

Choose a reason for hiding this comment

wzshiming Apr 9, 2023

Choose a reason for hiding this comment

nikola-jokic Apr 9, 2023

Choose a reason for hiding this comment

nikola-jokic Apr 9, 2023

Choose a reason for hiding this comment

nikola-jokic commented Apr 9, 2023

wzshiming commented Apr 12, 2023

k8s-ci-robot commented Apr 12, 2023

nikola-jokic commented Apr 12, 2023

nikola-jokic commented Mar 24, 2023 •

edited by wzshiming

Loading

netlify bot commented Mar 24, 2023 •

edited

Loading