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

rkt/api_service: add implementation for basic api service interfaces. #1508

Merged
merged 1 commit into from Oct 23, 2015

Conversation

Projects
None yet
5 participants
@yifan-gu
Contributor

yifan-gu commented Sep 30, 2015

This PR implements basic interfaces for reading pod/image information:
ListPods(), InspectPod(), ListImages(), InspectImage(). And GetInfo()
to get rkt/appc/api version info.

@yifan-gu

This comment has been minimized.

Show comment
Hide comment
@yifan-gu

yifan-gu Sep 30, 2015

Contributor

Need some feedbacks on the pod/image filtering semantics:

  • If filter includes multiple annotation/label pairs, should we combine them in AND or OR by default?
    (we could ask this for the user, but I'd rather we have a reasonable default behavior in this version).
  • Should we keep ImageNames in the pod filter, or should we return image name in ListPod()? As image name is not required in pod manifest, it can be empty. If we return the image name, we will need to do more disk I/Os.
Contributor

yifan-gu commented Sep 30, 2015

Need some feedbacks on the pod/image filtering semantics:

  • If filter includes multiple annotation/label pairs, should we combine them in AND or OR by default?
    (we could ask this for the user, but I'd rather we have a reasonable default behavior in this version).
  • Should we keep ImageNames in the pod filter, or should we return image name in ListPod()? As image name is not required in pod manifest, it can be empty. If we return the image name, we will need to do more disk I/Os.
@yifan-gu

This comment has been minimized.

Show comment
Hide comment
@yifan-gu

yifan-gu Sep 30, 2015

Contributor

dang, types.Labels composite literal uses unkeyed fields, This passes on my go 1.4.2..

Contributor

yifan-gu commented Sep 30, 2015

dang, types.Labels composite literal uses unkeyed fields, This passes on my go 1.4.2..

@jonboulle

This comment has been minimized.

Show comment
Hide comment
@jonboulle

jonboulle Oct 1, 2015

Contributor

@yifan-gu I am targeting this for the next release after 0.9.0 unless you shout otherwise

Contributor

jonboulle commented Oct 1, 2015

@yifan-gu I am targeting this for the next release after 0.9.0 unless you shout otherwise

@jonboulle jonboulle modified the milestones: v1.0.0, v0.10.0 Oct 2, 2015

@yifan-gu

This comment has been minimized.

Show comment
Hide comment
@yifan-gu

yifan-gu Oct 13, 2015

Contributor

Depending on #1604

Contributor

yifan-gu commented Oct 13, 2015

Depending on #1604

@jonboulle

This comment has been minimized.

Show comment
Hide comment
@jonboulle

jonboulle Oct 13, 2015

Contributor

ready for review?

Contributor

jonboulle commented Oct 13, 2015

ready for review?

@yifan-gu

This comment has been minimized.

Show comment
Hide comment
@yifan-gu

yifan-gu Oct 13, 2015

Contributor

@jonboulle Going to change the filtering rules a little bit.

Contributor

yifan-gu commented Oct 13, 2015

@jonboulle Going to change the filtering rules a little bit.

@yifan-gu

This comment has been minimized.

Show comment
Hide comment
@yifan-gu

yifan-gu Oct 13, 2015

Contributor

Ready for a review @jonboulle @alban @iaguis @eyakubovich

(Why there isn't things like @rkt-team?)

Contributor

yifan-gu commented Oct 13, 2015

Ready for a review @jonboulle @alban @iaguis @eyakubovich

(Why there isn't things like @rkt-team?)

@jonboulle

This comment has been minimized.

Show comment
Hide comment
@jonboulle

jonboulle Oct 13, 2015

Contributor

@yifan-gu you can create one if you like

Contributor

jonboulle commented Oct 13, 2015

@yifan-gu you can create one if you like

@yifan-gu

This comment has been minimized.

Show comment
Hide comment
@yifan-gu

yifan-gu Oct 14, 2015

Contributor

Working on functional tests, but will put in another PR

Contributor

yifan-gu commented Oct 14, 2015

Working on functional tests, but will put in another PR

Show outdated Hide outdated rkt/api_service.go Outdated
@yifan-gu

This comment has been minimized.

Show comment
Hide comment
@yifan-gu

yifan-gu Oct 17, 2015

Contributor

Ping?

Contributor

yifan-gu commented Oct 17, 2015

Ping?

Show outdated Hide outdated rkt/api_service.go Outdated
Show outdated Hide outdated rkt/api_service.go Outdated
Show outdated Hide outdated rkt/api_service.go Outdated
Show outdated Hide outdated rkt/api_service.go Outdated
Show outdated Hide outdated rkt/api_service.go Outdated
return &v1alpha.ListImagesResponse{Images: images}, nil
}
func (s *v1AlphaAPIServer) InspectImage(ctx context.Context, request *v1alpha.InspectImageRequest) (*v1alpha.InspectImageResponse, error) {

This comment has been minimized.

@krnowak

krnowak Oct 21, 2015

Collaborator

Missing docstring.

@krnowak

krnowak Oct 21, 2015

Collaborator

Missing docstring.

Show outdated Hide outdated rkt/api_service.go Outdated
return fmt.Errorf("not implemented yet")
}
func (s *v1AlphaAPIServer) ListenEvents(request *v1alpha.ListenEventsRequest, server v1alpha.PublicAPI_ListenEventsServer) error {

This comment has been minimized.

@krnowak

krnowak Oct 21, 2015

Collaborator

Missing docstring.

@krnowak

krnowak Oct 21, 2015

Collaborator

Missing docstring.

This comment has been minimized.

@yifan-gu

yifan-gu Oct 21, 2015

Contributor

I think it's ok as these docs are already in the interface definition. Copy-pasting them will create a burden on syncing the docs. @krnowak

@yifan-gu

yifan-gu Oct 21, 2015

Contributor

I think it's ok as these docs are already in the interface definition. Copy-pasting them will create a burden on syncing the docs. @krnowak

This comment has been minimized.

@krnowak

krnowak Oct 21, 2015

Collaborator

Ah, ok. Nevermind then about docstrings.

@krnowak

krnowak Oct 21, 2015

Collaborator

Ah, ok. Nevermind then about docstrings.

@@ -694,6 +694,20 @@ func (ds Store) GetAllACIInfos(sortfields []string, ascending bool) ([]*ACIInfo,
return aciInfos, err
}
func (ds Store) GetACIInfoWithBlobKey(blobKey string) (*ACIInfo, error) {

This comment has been minimized.

@krnowak

krnowak Oct 21, 2015

Collaborator

Missing docstring.

@krnowak

krnowak Oct 21, 2015

Collaborator

Missing docstring.

@yifan-gu

This comment has been minimized.

Show comment
Hide comment
@yifan-gu

yifan-gu Oct 21, 2015

Contributor

@alban @iaguis Why a killed app doesn't create status file? Should reaper.sh $app be invoked according to #1407 (comment)

Contributor

yifan-gu commented Oct 21, 2015

@alban @iaguis Why a killed app doesn't create status file? Should reaper.sh $app be invoked according to #1407 (comment)

@yifan-gu

This comment has been minimized.

Show comment
Hide comment
@yifan-gu

yifan-gu Oct 21, 2015

Contributor

It would be good to add a functional test that adds some images and pods and then talk to the api service on its socket to check them. Can you file a follow-up issue for this?

Will do after this pr

Contributor

yifan-gu commented Oct 21, 2015

It would be good to add a functional test that adds some images and pods and then talk to the api service on its socket to check them. Can you file a follow-up issue for this?

Will do after this pr

@yifan-gu

This comment has been minimized.

Show comment
Hide comment
@yifan-gu

yifan-gu Oct 21, 2015

Contributor

Need to think about the app state, and image info in inspectpod.

Contributor

yifan-gu commented Oct 21, 2015

Need to think about the app state, and image info in inspectpod.

@yifan-gu

This comment has been minimized.

Show comment
Hide comment
@yifan-gu

yifan-gu Oct 21, 2015

Contributor

I think testing the existence of the status file is not reliable enough, even combined with the pod's state. (e.g. pod can be running even if the app is aborted in the future)

I propose we hold a flock on the status dir during the execution of the app. So in order to determine the state of the app, we can:

  • try to lock the status dir (it must exist after rkt prepare)
    • if fails, the app is Running
    • if succeeds, try to read the exit code
      • if fails, the app is NotStarted (it can start later, or it's aborted before start for some reason)
      • if success, the app is Exited

But maybe we can just communicate the systemd in the nspawn to tell us..

Contributor

yifan-gu commented Oct 21, 2015

I think testing the existence of the status file is not reliable enough, even combined with the pod's state. (e.g. pod can be running even if the app is aborted in the future)

I propose we hold a flock on the status dir during the execution of the app. So in order to determine the state of the app, we can:

  • try to lock the status dir (it must exist after rkt prepare)
    • if fails, the app is Running
    • if succeeds, try to read the exit code
      • if fails, the app is NotStarted (it can start later, or it's aborted before start for some reason)
      • if success, the app is Exited

But maybe we can just communicate the systemd in the nspawn to tell us..

@yifan-gu

This comment has been minimized.

Show comment
Hide comment
@yifan-gu
Contributor

yifan-gu commented Oct 21, 2015

@alban

This comment has been minimized.

Show comment
Hide comment
@alban

alban Oct 22, 2015

Member

@alban @iaguis Why a killed app doesn't create status file? Should reaper.sh $app be invoked according to #1407 (comment)

If systemd in the pod is terminated immediately rather than a normal shutdown (e.g. the user presses ^]^]^] in the terminal), systemd cannot start reaper.sh.

Member

alban commented Oct 22, 2015

@alban @iaguis Why a killed app doesn't create status file? Should reaper.sh $app be invoked according to #1407 (comment)

If systemd in the pod is terminated immediately rather than a normal shutdown (e.g. the user presses ^]^]^] in the terminal), systemd cannot start reaper.sh.

@krnowak

This comment has been minimized.

Show comment
Hide comment
@krnowak

krnowak Oct 22, 2015

Collaborator

Please rebase it - it will pickup fixes for semaphore (installing the vet and cover tools).

Collaborator

krnowak commented Oct 22, 2015

Please rebase it - it will pickup fixes for semaphore (installing the vet and cover tools).

@alban

This comment has been minimized.

Show comment
Hide comment
@alban

alban Oct 22, 2015

Member

I propose we hold a flock on the status dir during the execution of the app.

If the api-service does a trylock and the pod does a regular lock, the api-service has the capability to delay indefinitely the startup of a pod because advisory locks don't need write access to be taken. @vcaputo is it correct?

Member

alban commented Oct 22, 2015

I propose we hold a flock on the status dir during the execution of the app.

If the api-service does a trylock and the pod does a regular lock, the api-service has the capability to delay indefinitely the startup of a pod because advisory locks don't need write access to be taken. @vcaputo is it correct?

Show outdated Hide outdated rkt/api_service.go Outdated
@alban

This comment has been minimized.

Show comment
Hide comment
@alban

alban Oct 22, 2015

Member

@yifan-gu I suggest to add a "FIXME" in this PR about the app status and then fix it in a future PR.

Member

alban commented Oct 22, 2015

@yifan-gu I suggest to add a "FIXME" in this PR about the app status and then fix it in a future PR.

return v1alpha.PodState_POD_STATE_EXITED
case Garbage:
return v1alpha.PodState_POD_STATE_GARBAGE
default:

This comment has been minimized.

@jonboulle

jonboulle Oct 22, 2015

Contributor

this is unnecessary, fyi

@jonboulle

jonboulle Oct 22, 2015

Contributor

this is unnecessary, fyi

This comment has been minimized.

@yifan-gu

yifan-gu Oct 22, 2015

Contributor

i'd like to keep a default..

@yifan-gu

yifan-gu Oct 22, 2015

Contributor

i'd like to keep a default..

Show outdated Hide outdated rkt/api_service.go Outdated
@yifan-gu

This comment has been minimized.

Show comment
Hide comment
@yifan-gu

yifan-gu Oct 22, 2015

Contributor

Added FIXME, rebased.

Contributor

yifan-gu commented Oct 22, 2015

Added FIXME, rebased.

Show outdated Hide outdated rkt/api_service.go Outdated
@jonboulle

This comment has been minimized.

Show comment
Hide comment
@jonboulle

jonboulle Oct 23, 2015

Contributor

@yifan-gu one (important) comment then LGTM

Contributor

jonboulle commented Oct 23, 2015

@yifan-gu one (important) comment then LGTM

@jonboulle

This comment has been minimized.

Show comment
Hide comment
@jonboulle

jonboulle Oct 23, 2015

Contributor

let's kick the tyres on this!

Contributor

jonboulle commented Oct 23, 2015

let's kick the tyres on this!

rkt/api_service: add implementation for basic api service interfaces.
This PR implements basic interfaces for reading pod/image information:
ListPods(), InspectPod(), ListImages(), InspectImage(). And GetInfo()
to get rkt/appc/api version info.
@jonboulle

This comment has been minimized.

Show comment
Hide comment
@jonboulle

jonboulle Oct 23, 2015

Contributor

merge on semaphore green
@yifan-gu please file a follow up issue for functional tests

Contributor

jonboulle commented Oct 23, 2015

merge on semaphore green
@yifan-gu please file a follow up issue for functional tests

@alban

This comment has been minimized.

Show comment
Hide comment
@alban

alban Oct 23, 2015

Member

👍

Member

alban commented Oct 23, 2015

👍

alban added a commit that referenced this pull request Oct 23, 2015

Merge pull request #1508 from yifan-gu/api_basic
rkt/api_service: add implementation for basic api service interfaces.

@alban alban merged commit 7499ac9 into rkt:master Oct 23, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment