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

add manifest type #52570

Closed
wants to merge 2 commits into from
Closed

add manifest type #52570

wants to merge 2 commits into from

Conversation

mengqiy
Copy link
Member

@mengqiy mengqiy commented Sep 15, 2017

Adding manifest type is the very first step develop declarative application management: https://docs.google.com/document/d/1cLPGweVEYrVqQvBLJg6sxV-TrE5Rm2MNOBA_cxZP2WU/edit#heading=h.dr88tktf0e99

This is the 1st version of manifest type that is directly translate from the proposal above.
We can always change it, since it is not a real API and it's alpha. It will be used purely client-side (at least for now).

Partially addresses: #52881

add type package and manifest in kubectl for base/overlay workflow model

/assign @monopole
cc: @bgrant0607 @pwittrock

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 15, 2017
@k8s-github-robot k8s-github-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 15, 2017
Copy link
Contributor

@monopole monopole left a comment

Choose a reason for hiding this comment

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

just to get started....

limitations under the License.
*/

// Package v1alpha1 contains the type definition for package.
Copy link
Contributor

Choose a reason for hiding this comment

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

Package

// An pointer to the icon.
Icon string `json:"icon,omitempty"`

// Maintainers list.
Copy link
Contributor

Choose a reason for hiding this comment

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

People that maintain this package.

Home string `json:"home,omitempty"`

// All sources where you can find this application package.
Sources []string `json:"sources,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest making this singular

Source string

so user not forced to code for an array and wonder about ordering / which entry is best.

If strongly desired, add secondary field

SourceMirrors []string

to allow specification of (alleged) mirrors of Source.

Also, suggest the following field commentary:

// Source specifies the URL, e.g. https://github.com/foo/bar.git, file://host/path, etc.
// hosting the resource files specified in Base and Overlays.
Source string

// A Base entry specifies the file path, relative to Source, of an API resource definition.
Bases []string

// An Overlay entry specifies the file path, relative to Source, of a file
// containing a Strategic Merge Patch overlay in API resource form.
Overlay []string

@mengqiy mengqiy force-pushed the manifest_type branch 3 times, most recently from 843d693 to ad2ff01 Compare September 18, 2017 20:42
@mengqiy
Copy link
Member Author

mengqiy commented Sep 18, 2017

There is still one remaining comment from @monopole in #52570 (comment). It is shown as outdated due to I updated something.
Summarize his comment:
Using

Source string
SourceMirrors []string

instead of

Sources []string

For the detail of the suggestion, please look at #52570 (comment).

@0xmichalis
Copy link
Contributor

@kubernetes/sig-cli-api-reviews @kubernetes/sig-apps-api-reviews

@k8s-ci-robot k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/apps Categorizes an issue or PR as relevant to SIG Apps. labels Sep 19, 2017
NamePrefix string `json:"namePrefix,omitempty" yaml:"namePrefix,omitempty"`

// Description of the application package.
Description string `json:"description,omitempty" yaml:"description,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Please put this kind of package "metadata" directly under ObjectMeta, above.

Copy link
Member

Choose a reason for hiding this comment

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

Proposed order:

  1. Description
  2. Icon
  3. Keywords
  4. Home
  5. Source
  6. Maintainers (I could also imagine dropping this)
  7. AppVersion

Then non-metadata fields.

  1. NamePrefix
  2. ObjectLabels
  3. ObjectAnnotations
  4. Bases
  5. Overlays
  6. ConfigMaps
  7. Secrets
  8. OwnPersistentVolumeClaims
  9. Recursive
  10. Prune

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Actually, how about we put the package metadata into a separate resource?

The metadata would drive package searching and browsing, and fork/rebase upgrade workflows. It might need to be consumed by app registry and other tools.

The non-metadata fields would drive kubectl resource processing functionality.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, would be better to have 2 distinct resources between metadata and data used by the tools.

Home string `json:"home,omitempty" yaml:"home,omitempty"`

// Sources specify the URLs, e.g. https://github.com/foo/bar.git, file://host/path, etc.
// hosting the resource files specified in Base and Overlays.
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment that this is intended to support fork/rebase workflows.

@bgrant0607 bgrant0607 self-assigned this Sep 20, 2017
Name string `json:"name,omitempty" yaml:"name,omitempty"`

// Maintainer email.
Email string `json:"email,omitempty" yaml:"email,omitempty"`
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 drop email due to privacy concerns.

// People that maintain this package.
Maintainers []Maintainer `json:"maintainers,omitempty" yaml:"maintainers,omitempty"`

// A Base entry specifies the file path, relative to Source, of an API resource definition.
Copy link
Member

Choose a reason for hiding this comment

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

These strings should be in whatever form kubectl's --filename parameter accepts. So at least files and directories. I recommend relative paths within the package, so that they'll work no matter how the bundle of files is delivered: fork from github, tarball from registry, curled from object storage, etc. URLs: Have to think more about that. As mentioned in #24649, I'd also like to support globs here.

Explicit individual files would be useful in the case that the package were retrieved directly via HTTP (though we shouldn't require that) or other yaml/json files were present in directories containing the package files.

Copy link
Member

Choose a reason for hiding this comment

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

How do you plan to process send those files to the API ? as separate API call for each resources?
I assume that expansion, generation, transformation, patches are done client-side, in that case what about creating a new field 'resources' to capture that ?
Server-side would apply/create from this field (like kind: list)

Copy link
Member Author

Choose a reason for hiding this comment

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

URLs: Have to think more about that.

Yes. I have concern of it as well. The underlying content of a URL may change and the user may not know. IMO it's not safe for fork/rebase workflow.

How do you plan to process send those files to the API ? as separate API call for each resources?
I assume that expansion, generation, transformation, patches are done client-side, in that case what about creating a new field 'resources' to capture that ?
Server-side would apply/create from this field (like kind: list)

Current plan is to have a expand command for prototyping. It will yield the api resources after applying the overlay on top of base. I imagine that it can be output to stdout or a file the resources are separated by --- (I think it is easier than using a list).

I will push more change to #52682 soon.

// containing a Strategic Merge Patch overlay in API resource form.
Overlays []string `json:"overlays,omitempty" yaml:"overlays,omitempty"`

// List of configmap overlays.
Copy link
Member

Choose a reason for hiding this comment

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

These aren't overlays, but are configmaps to generate from configuration sources. Overlays in ConfigMap API syntax would just go in the Overlays list. Ditto for Bases.

// List of configmap overlays.
Configmaps []ConfigMap `json:"configmaps,omitempty" yaml:"configmaps,omitempty"`

// List of secret overlays.
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as for ConfigMaps

// The type of the configmap. e.g. `env` and `file`.
Type string `json:"type,omitempty" yaml:"type,omitempty"`

// Partial name that will prefix the configmap overlays.
Copy link
Member

Choose a reason for hiding this comment

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

Look at kubectl create configmap --append-hash to understand what parameters are needed and what they mean

Type string `json:"type,omitempty" yaml:"type,omitempty"`

// Partial name that will prefix the secret overlays.
NamePrefix string `json:"namePrefix,omitempty" yaml:"namePrefix,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as for ConfigMap

Keywords []string `json:"keywords,omitempty" yaml:"keywords,omitempty"`

// Version of the application package.
AppVersion string `json:"appVersion,omitempty" yaml:"appVersion,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

It seems that users would like 2 versions fields:
From the Chart.yaml spec:

version: A SemVer 2 version (required)
appVersion: The version of the app that this contains (optional). This needn't be SemVer.

cc @prydonius

Copy link
Member

Choose a reason for hiding this comment

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

@ant31 The standard Kubernetes API metadata already contains a version for the schema of this specification.

Copy link
Member

Choose a reason for hiding this comment

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

Yes spec are versioned.
I was talking about the configuration content.

e.g User create a first iteration of a 'postgres' package:

appVersion: 9.6.0
packageVersion: 1.0.0
bases:
 - deployment.yaml

and then later modify the package, to add a configmap

appVersion: 9.6.0          
packageVersion: 1.1.0   // upgrade the package version
bases:
 - deployment.yaml
configMaps: 
- type: env
  namePrefix: app-env
  file: app.env

Both deploy a postgres 9.6.0 (appVersion), but the configuration iterations would be tracked via the 'packageVersion' (version in case of Chart. )

Helm, app-registry indexes packages base on the 'packageVersion' and not the 'appVersion', because it's the kubernetes-resources that are deployed and each changes should be tracked.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. We should just start with packageVersion rather than appVersion, then.

The image tag and/or hash is likely to be one of the most frequently customized properties, and any specification in the package metadata would impact both the customization strategy and the update model.

So let's omit appVersion for now.

Choose a reason for hiding this comment

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

+1 though it's worth mentioning that an appVersion may be dependent on a packageVersion, for example if a new required env var is introduced in an app and the package must be updated to support it.

// Package could be either a base or a overlay manifest.
type Package struct {
// Metadata for package searching and browsing, and fork/rebase workflow.
Meta PackageMeta `json:"meta,omitempty" yaml:"meta,omitempty"`
Copy link
Member Author

@mengqiy mengqiy Sep 21, 2017

Choose a reason for hiding this comment

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

Do we want to inline PackageMeta or not? I'm not sure.

@mengqiy
Copy link
Member Author

mengqiy commented Sep 21, 2017

Discussed with @bgrant0607 offline, and updated the PR:

  • Split it into 2 types: package and manifest.
  • Add name, description and a separate version for each type.
  • Update some comments to make it more clear (hopefully).

The structure of the file will change a bit:

/Kube-package.yaml # Use Package to represent

/package/Kube-manifest.yaml # Use Manifest to represent
/package/base.yaml
/package/...

/instances/variant1/Kube-manifest.yaml
/instances/variant1/overlay.yaml
/instances/variant1/...

/instances/variant2/...
/instances/variant3/...

@mengqiy
Copy link
Member Author

mengqiy commented Sep 22, 2017

Minor comments updates for #52570 (review) and squashed commits.

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2017
@mengqiy
Copy link
Member Author

mengqiy commented Sep 22, 2017

Is there a specific issue for this?

Created an issue #52881 for now.
I believe Jeff has doc of a list of tasks, but it is not ready to share now. It should be ready before next sig-cli meeting.

@mengqiy
Copy link
Member Author

mengqiy commented Sep 22, 2017

Re. where should the command live.
This should at least have some value as Brian mentioned above.
I don't find a better place with little overhead to setup.
Furthermore, we can hide that command (supported by Cobra) if we want, so the user won't navigate to it.

// TLS secret.
TLS TLS `json:"tls,omitempty" yaml:"tls,omitempty"`

// DockerRegistry DockerRegistry `json:"dockerRegistry,omitempty" yaml:"dockerRegistry,omitempty"`
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we support DockerRegistry secret?

KeyFile string `json:"keyFile,omitempty" yaml:"keyFile,omitempty"`
}

//type DockerRegistry struct {
Copy link
Member Author

Choose a reason for hiding this comment

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

DockerRegistry contains sensitive info.

Copy link
Member

Choose a reason for hiding this comment

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

Good point.

Assuming we were going to support multiple secret types with different arguments (e.g., generic), though, how should we structure this?

@k8s-ci-robot
Copy link
Contributor

@mengqiy: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-etcd3 1465cf0 link /test pull-kubernetes-e2e-gce-etcd3

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.


// Whether prune resources not defined in Kube-manifest.yaml, similar to `kubectl apply --prune` behavior.
Prune bool `json:"prune,omitempty" yaml:"prune,omitempty"`
}
Copy link
Member

Choose a reason for hiding this comment

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

how manifest and package resources linked together ?

Copy link
Member

Choose a reason for hiding this comment

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

They are not explicitly linked at present. They have different consumers. The Package information would be consumed by registry tools. The Manifest information would be consumed by kubectl and/or other configuration tools. I'd expect both to be present in the same directory tree. However, in the presence of overlays, nested packages, and other scenarios, there would be multiple Manifest instances in the tree.

Copy link
Member

Choose a reason for hiding this comment

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

Also note that they don't have to be instantiated as resources. But are structured such that they could be.

Copy link
Contributor

@monopole monopole Sep 25, 2017

Choose a reason for hiding this comment

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

I started several comments then deleted them all
as i realized they were trying to paper over
trouble with Package and Manifest
per my own experience with other manifest schemes.

Some examples of wildey used manifests: android, chrome, microsoft, and apple plist file (generally hidden by Xcode). Each contain

  • app-descriptor fields - icon, prose description, version, etc.
  • app-operation fields - activities, services, etc.

Observations:

  1. If app-operation fields are deleted, app-descriptor fields serve no purpose.
    Both are components of the manifest, not independent things, even though they
    are obviously consumed differently. Likewise, an app-descriptor is never
    attached to multiple sets of app-operation fields. DAM is no different.
    Separating app-descriptor fields into something with a distinct lifecycle
    only creates a disassociation problem.

  2. Obviously these manifests describe an app "instance". Snapchat's manifest is
    not intended as a template from which to create new chat apps. The DAM
    manifest, OTOH wants to deliver information allowing developers to maintain N
    different instances. Nevertheless, each instance is rooted in a cloned base
    that is periodically rebased. One manifest still makes sense.

Pondering the manifests already out there, i suggest these changes:

  1. Follow convention - make Manifest the top level thing.

  2. Description fields should stay in the Manifest.

    It's fine to gather them in a struct for convenience, but call that
    struct something that doesn't cognitively collide with Go and Java's
    package.

  3. Manifest and Overlay are different things.

    K8s Config is easy to explain in linear algebra terms if we present a
    manifest as a state vector, and an overlay as a matrix to apply to that
    vector. The meaning of an overlay, and the possiblity of chaining them
    becomes obvious.

    Brian's initial proposal to have two different types is less confusing and
    easier to validate.

  4. Simplify file layout.

    Reasons discussed below in terms of development lifecycle.

  5. Drop the recursive field.

    The notion of a file tree with Manifest and Overlay files sprinkled around in
    it needs clear explantion else it should be treated as bad input. For
    example, in what order does one apply disovered overlays? How does one avoid
    surprises (e.g. like the surprise --prune already provides).

Structs:

  // Manifest contains everything needed to create a k8s cluster.
  type Manifest struct {
     <standard k8s resource fields>

     // Metadata about the app.   <-- name "metadata" taken by k8s.  Eschew
     descriptor Descriptor        <-- "Package" because GoLang cognitive overload.

     // Paths (relative to manifest location) to k8s resource YAML files
     // that constitute the initial cluster state.  The files are present
     // in the manifest distribution, and can be recovered if misplaced by,
     // say, git checkout.  Other files found in the same tree yet not
     // specified here may belong to overlays, or may simply be added to
     // the cluster.
     Resources []string

     // Generators of ConfigMap and Secret instances from env files, etc.
     ConfigMapGenerators []ConfigMapGenerator
     SecretGenerators []SecretGenerator
  }

  // Descriptor is a convenience struct gathering metadata to support
  // Manifest search, browse and updates.
  type Descriptor struct {
     <standard k8s resource fields>
     Name string
     // Change version when a resource added, but not, say when a container
     // image changes. This field maintained by the manifest author.
     Version string
     Description string
     Icon string
     Keywords []string
     Homepage string
     ...
  }

  // Overlay describes changes to make to a Manifest or the
  // result of an Overlay applied to a Manifest.
  type Overlay struct {
     <standard k8s resource fields>

     // Evolves separately from Manifest version.
     // This field maintained by the owner of the overlay.
     Version string

     // Path to either another Overlay or a Manifest file.
     // Clients like kubectl will follow these links.
     // If unspecified, defaults to './Kube-manifest.yaml'
     Operand string

     NamePrefix string
     ObjectLabels map...
     ObjectAnnotations map...

     // Resources to add.  Delete a resource by just deleting it from the tree.
     Resources []string

     // Files consumable as a strategic-merge-patch.
     Smps []string

     OwnPvc bool
     Prune bool  // needs clear explanation
     // Recursive bool -- Dropped until we can agree what it means.
  }

Then this simple arrangement in the Manifest tarball/githubrepo:

  Kube-manifest.yaml   // Instance of Manifest
  LICENSE
  README.md
  identity-overlay.yaml // Provided as a convenience - a NO-OP overlay.
  resources/
    deployment.yaml
    ingress.yaml

Finally (no different from Brian's plan), the end user

  1. Clones the upstream repo after finding it in the store.
  2. Edits Kube-manifest.yaml to suit their purposes.
  3. Optionally renames identity-overlay.yaml to
    my-overlay.yaml and adds any resources the overlay
    needs to the resources/ directory.
  4. Checks everything in to their own repo for
    periodic rebasing against upstream.
  5. Runs kubectl apply -f my-overlay.yaml

Copy link
Member Author

Choose a reason for hiding this comment

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

identity-overlay.yaml // Provided as a convenience - a NO-OP overlay.

I think this tree structure may encourage the users to put a bunch of overlay k8s API resources in one single file, which may not be a good practice to manage different kinds of resources.

Copy link
Member

@bgrant0607 bgrant0607 Sep 26, 2017

Choose a reason for hiding this comment

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

@monopole

Thanks for the manifest examples.

I like "Descriptor" for the package metadata. I wouldn't want an application registry to look at the other manifest fields, though.

Embedded structures shouldn't generally contain K8s API resource fields. Do you envision generating CRDs from those embedded resources?

One usage model that I'd like to support is the following:

  • Fork a package that contains base resources
  • Create multiple variants, implemented using overlays, additional resources, and variant-specific name prefixes and labels

Another is:

  • Fork a package that contains base resources
  • Customize it by mutating the base resources, adding additional resources (e.g., secrets), and specifying a name prefix and labels
  • Rebase to upgrade the bases

Another is:

  • I just want to create declarative instructions for kubectl so that I can update my app with one command instead of a shell script, and make it impossible to forget to specify the appropriate label selector with apply --prune

It is common in Borg to put directories for variants and the directory for the base templates at the same level in the directory hierarchy. That could work. But I think it get confusing for files corresponding to multiple variants to go into the same subdirectory. Either way, though we should recommend a convention, I think we should support syntax that leaves the structure up to the user.

I'm fine with omitting the recursive option for now.

Copy link
Member

Choose a reason for hiding this comment

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

In your proposal, an overlay is always required, correct?

Also, the configmap and secret generators would need to be added to Overlay.

Copy link
Member

Choose a reason for hiding this comment

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

Do you envision generating CRDs from those resources?

Yes,
PackageMetadata could be used by other workflow managers. It doesn't have to refer exclusively to this new Manifest type. I would keep app-operation and app-descriptor separated.

Can we reference PackageMetadata resource from the Manifest?

     descriptor: String       <-- " PackageResource name”

Manifest and Overlay are different things.

How are they different and what features would it add to separate them ?

IMO, in your proposal Overlays could be seen as a Manifest instance.
In that case, having an Overlay (even empty) is a requirement and it became the main resource in the cluster.

kubectl apply -f overlay.yaml
kubectl get overlays
kubectl delete overlay foo #  cascade delete

I’d prefer to keep them optional and only a way to declare small variations when templating / transformations aren’t enough.

the possiblity of chaining them becomes obvious.

Do we need to support chaining overlays, it would increase complexity for the user and become more fragile.
From Brian’s declarative app:

I do not think we should support arbitrary numbers of stacked overlays. [...] changing a base layer invalidates layers stacked on top of it. Beyond a couple layers it also becomes hard to reason about the implications of each overlay.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ant31 There are detailed examples of why Manifest and Overlay are different. The Manifest is a state you download and edit as desired, the Overlay is an operator - it drives the cluster represented by the manifest to a new state via mechanisms that cannot be achieved by editing the manifest directly. It's also the way you get to N variations on the base state. One editted manifest is just one state (N==1). A manifest plus one overlay is still just N==1. Also, the Manifest is registered in a store, the Overlay is not. One could register the overlay in a store, but it wouldn't be an independent thing - it points to a manifest.

@bgrant0607 and @ant31 - Yes, this supports those three goals.

Fork a package that contains base resources. Create multiple variants..
Fork the repo for the manifest, create overlay1, overlay2, etc. Run

  • kubectl apply -f overlay1.yaml // internal to this file is a pointer to Kube-manifest.xml
  • kubectl apply -f overlay2.yaml // ditto
  • ...

Fork a package, customize it by mutating the base resources....

Fork the repo for the manifest, edit as desired, add new resources, don't create overlays. Run

  • kubectl apply -f Kube-Manifest.yaml

I just want to create declarative instructions for kubectl

Create a manifest and apply it.

@bgrant0607

In your proposal, an overlay is always required, correct?

To get the name-prefix, alas, yes. We could embed the "first" overlay directly in the manifest, and i initially wrote it up that way, and then found that the whole thing was much harder to explain. You have to say stuff like: The overlay embedded in the manifest is applied before any overlays from the outside are applied. weird.

Brian's right to presume one wouldn't chain overlays in practice. However, we either create a means to do it sensibly, or we should prohibit it entirely. Clearly the manifest is a state, clearly the overlay modifies that state. You almost have to go out of your way to prohibit the application of another overlay.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, Brian and I have had another chat. I think my objections have been addressed, and i can get behind conflating what i was calling Manifest and Overlay, and that @ant31 is right in claiming they are the same(-ish). Descriptor the name seems to have survived the discussion as a replacement for Package, and we can add a field to Manifest to allow one to find its Descriptor. We'll also add more description to pin down how one, when presented with a directory tree containing manifest files, arrives at a single unambiguous order of expansion.

@bgrant0607
Copy link
Member

Some SIG testing folks told me just now that it would be "trivial" to set up tests for a new branch. So I think the main objection to a feature branch is no longer valid.

@mengqiy
Copy link
Member Author

mengqiy commented Sep 23, 2017

Some SIG testing folks told me just now that it would be "trivial" to set up tests for a new branch. So I think the main objection to a feature branch is no longer valid.

Will move it to the feature branch. Just talked to one of the sig-testing folks, setup presubmit tests should be trivial, but setup CI tests is not. But IMO presubmit should be enough for now.

@smarterclayton Are you OK to let us do it in a feature branch?

@smarterclayton
Copy link
Contributor

I'm going to be pretty firm on this - we have stated that large experiments be conducted using our extension mechanisms both as proof that we are delivering features incrementally, proving value before dropping large changes into kube, and to prove the extensions work.

Binary plugins seems perfectly suitable for experimenting with this. I don't see a need to do it in tree.

@smarterclayton
Copy link
Contributor

smarterclayton commented Sep 25, 2017

A feature branch doesn't feel like it accomplishes two of those points - which are proving features in a way that force us to behave like we are extending the core platform (even if those features may eventually become core) nor does it leverage the extension mechanisms. Feature branches are useful, but this is:

  1. a wholly new "feature" that has little or no coupling to the core kube code except reuse
  2. is a CLI only area for now
  3. could easily be a binary plugin that could iterate at a completely different lifecycle than something in tree.

There needs to be an incredibly strong case that this active experimentation MUST happen in kubernetes/kubernetes, otherwise we are saying that some big changes are ok to prototype in core and some are not, which is very difficult to consistently apply to the entire community.

@0xmichalis
Copy link
Contributor

+1 on modeling this as a kubectl plugin and starting with a separate repo for it. It will be easier to track its development because enabling notifications for the main repo is simply not working.

@0xmichalis
Copy link
Contributor

We can still use all of the CI tooling with a separate repo. I can work with sig-testing folks and set up a job for it to run builds+tests.

@bgrant0607
Copy link
Member

kubectl itself should be able to iterate on a completely separate cycle from kubernetes/kubernetes, but I can find another project to try feature branches.


// A Base entry specifies the relative paths within the package.
// It could be any format that kubectl -f allows, i.e. files, directories, URLs and globs.
Bases []string `json:"bases,omitempty" yaml:"bases,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Please rename this to resources

// An Overlay entry is very similar to an Base entry.
// It specifies the relative paths within the package, and could be any format that kubectl -f allows.
// It should be able to be merged by Strategic Merge Patch on top of its corresponding Base if any.
Overlays []string `json:"overlays,omitempty" yaml:"overlays,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Please rename this to patches.

Secrets []Secret `json:"secrets,omitempty" yaml:"secrets,omitempty"`

// Whether PersistentVolumeClaims should be deleted with the other resources.
OwnPersistentVolumeClaims bool `json:"ownPersistentVolumeClaims,omitempty" yaml:"ownPersistentVolumeClaims,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

I'd be fine with making this a TODO

OwnPersistentVolumeClaims bool `json:"ownPersistentVolumeClaims,omitempty" yaml:"ownPersistentVolumeClaims,omitempty"`

// Whether recursive look for Kube-manifest.yaml, similar to `kubectl --recursive` behavior.
Recursive bool `json:"recursive,omitempty" yaml:"recursive,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

I'd be fine with making this a TODO until we work out the details

@ant31
Copy link
Member

ant31 commented Sep 28, 2017

About parameters, without endorsing any solution we can give a path forward to build it on top and improve interoperability of tools in the ecosystem.

Most of the tool are using similar way to specify variables and we could normalize that in the manifest

       //  Variables dict to be used by plugins / crd
       Variables map...
       // and/or or via a file 	
       VariablesFile string

Other part definition of the plugin:
I assume one plugin per manifest

type Expander struct {
     Name string
     Cmd []string
     Image string 
} 

type Manifest struct {
     variablesFile string
     expander Expander 
      ...
}

This way upstream is still agnostic and not endorse any template engine/dsl but still define a pattern to follow in the ecosystem. As an user consuming/deploying an application, the UX will be the same regardless of the templating used jsonnet / jinja2 / erb / gotpl / helm

@mengqiy
Copy link
Member Author

mengqiy commented Sep 28, 2017

Reopened this PR with minor updates kubernetes/kubectl#64 in kubectl repo.

@bgrant0607
Copy link
Member

@ant31

Parameters are out of scope of this effort. Even if they weren't the Descriptor and Manifest aren't the right places to put them. That would not be compatible with any existing parameter-substitution mechanisms. Even if the Manifest were the right place to put parameters, that requires a bigger design effort. Most parameterization mechanisms, including gotpl, don't adequately specify the parameter schema, for instance. In comparison, look at CRD validation.

@bgrant0607
Copy link
Member

This has moved.

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants