8000 Add getGCMarkedTime() and return the timestamp in rkt api service. by yifan-gu · Pull Request #2402 · rkt/rkt · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Feb 24, 2020. It is now read-only.

Add getGCMarkedTime() and return the timestamp in rkt api service. #2402

Merged
merged 4 commits into from
Apr 14, 2016

Conversation

yifan-gu
Copy link
Contributor
@yifan-gu yifan-gu commented Apr 9, 2016

Follows #2400

This PR adds getGCMarkedTime(), which when is called, it will return the ctime of the pod's dir if the pod is in exited-garbage or garbage dir.
If the pod is running, not starting or gone, it returns zero.

Also updates the rkt api service test to test the GCMarkedTime is returned correctly.

cc @vcaputo @jonboulle @euank @sjpotter @alban @iaguis

@@ -770,6 +771,8 @@ func (p *pod) getModTime(path string) (time.Time, error) {
return time.Time{}, err
}

defer f.Close()
Copy link
Member

Choose a reason for hiding this comment

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

move this to right after you open the file?

@yifan-gu
Copy link
Contributor Author

@iaguis @alban @vcaputo @sjpotter @euank

Hmm, I added some changes to the rkt api service test today and expected it to pass. However I found it was not the case:
rkt api service runs as gid=rkt-group if possible, it means it doesn't have the write permit to move the pod from /run to /exited-garbage.

Same thing could happen when rkt list runs as non-root. So I guess maybe in rkt list/status/api-service, we could not move the pod...

Ideas?

@iaguis
Copy link
Member
iaguis commented Apr 12, 2016

I think we can only mark the pod as finished on rkt gc --mark-only if we want to keep this non-root thing.

I think showing the finished time in rkt list will be confusing for users, it will be empty unless they mark the pod with gc...

@yifan-gu
Copy link
Contributor Author

@iaguis OOB discussed with @euank a little bit. Another idea would be adding a flag (e.g. --show-gc-info) to rkt list/status, when the flag is added, then it will require root and show the Finished time.
When rkt api service is running as root, return Finished time, if not, don't return the finished time...

I am not totally happy with all there options..

@euank
Copy link
Member
euank commented Apr 12, 2016

We could make this feel slightly less wrong by not calling it finished anywhere, but rather marking something like gcTime, gcStatus or such. I think that at least makes it less confusing. We can then overload that to mean finished only in rktnetes where we know there's an ExecStart=rkt gc $id.

It also makes sense to me to gate it behind a flag (e.g. rkt list --gc-status). Gating it especially makes sense given the root / non-root differentiation.

This does make the feature less obvious, but I'm also not sure that's a real issue.
To me, this seems like the best option if we want to continue down the gc-path for finished time.

@yifan-gu
Copy link
Contributor Author

@euank gctime or gc-marked-time sounds good to me. so if the pod is not gc marked, then just return nothing.

We can also just return this in api service. We do that for the cgroup-path already.

@yifan-gu
Copy link
Contributor Author

Maybe for this one, just make rkt api service to return gc time.
Then we open an issue discussing how to handle the rkt list/status in another issue? @euank

Yifan Gu added 2 commits April 12, 2016 17:13
This returns the timestamp when the pod is moved to garbage/exited-garbage.
@yifan-gu yifan-gu changed the title Add getFinishTime() and return the timestamp in rkt list/status and api service. Add getGCMarkedTime() and return the timestamp in rkt api service. Apr 13, 2016
@yifan-gu
Copy link
Contributor Author

Updated the PR and description message.

gcCmd := fmt.Sprintf("%s gc --mark- ctx.Cmd())
waitOrFail(t, spawnOrFail(t, gcCmd), 0)

gcTime := time.Now()
Copy link
Member

Choose a reason for hiding this comment

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

Since compareTime checks if the times are equal within a 2 second precision, I think the test would be more meaningful if we wait some time before marking the pod. In the rkt list test we wait 4 seconds.

Copy link
Member

Choose a reason for hiding this comment

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

Let's address this on #2420

@iaguis
8000 Copy link
Member
iaguis commented Apr 14, 2016

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0