Skip to content
This repository has been archived by the owner on Aug 14, 2020. It is now read-only.

Update appc data structures for CRI #656

Merged
merged 3 commits into from
Sep 20, 2016
Merged

Update appc data structures for CRI #656

merged 3 commits into from
Sep 20, 2016

Conversation

squeed
Copy link
Contributor

@squeed squeed commented Sep 19, 2016

@yifan-gu @lucab @s-urbaniak I also updated the appc spec for annotations, along with port forwards. Take a look and we can update glide to pull from appc/spec's CRI branch.

@squeed
Copy link
Contributor Author

squeed commented Sep 19, 2016

@yifan-gu this supersedes #655, with a slightly cleaner interface. Sorry for stepping on your toes.

@lucab
Copy link
Contributor

lucab commented Sep 19, 2016

For reference, this PR goes to a new cri branch which is aimed at being a playground and an early opportunity for reviewing changes related to supporting the new Kubernetes CRI runtime interface CRI includes a slightly different handling of pods and apps, and work is ongoing on the rkt side to support it. It looks fine to me, but I have no superpowers here.

Copy link
Contributor

@yifan-gu yifan-gu left a comment

Choose a reason for hiding this comment

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

OK with the plan for implementing on a feature branch, with just one question on annotaion/label.

Isolators []types.Isolator `json:"isolators"`
Annotations types.Annotations `json:"annotations"`
Ports []types.ExposedPort `json:"ports"`
CRIAnnotations types.CRIAnnotations `json:"criAnnotations,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

@squeed CRIAnnotation and CRILabels sounds too specific for CRI? Why not just using the old Annotations and changing the type of the Key from ACIdentifier to string?
Both Label or Annotation can be muxed into annotations with prefixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yifan-gu I thought about that, but the appc spec already defines some app annotations with special parsing rules, e.g. 'created'.
If we're going to add a type that allow arbitrary user-specified Kubernetes labels, we can't go adding more meaning to them.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be inline with previous discussion about this at rkt/rkt#3010 (comment)

@squeed
Copy link
Contributor Author

squeed commented Sep 20, 2016

We need to make progress on other parts of the CRI push, so I'm merging this, but we don't need to stop the discussion.

@yifan-gu
Copy link
Contributor

yifan-gu commented Sep 21, 2016

@squeed We also need to add the same fields in the app level.

@squeed NVM, I saw them in the RuntimeApp.App...


import "fmt"

type CRILabels map[ACIdentifier]string
Copy link
Contributor

@yifan-gu yifan-gu Sep 21, 2016

Choose a reason for hiding this comment

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

@squeed Why labels keys are not arbitrary strings as well? Today, the labels injected by kubelet is not ACIdentifier compliant.
Although there is a TODO to change that, and they do smell like annotations then labels.

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