Skip to content

Add apis for machine-class #488

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Oct 22, 2018
Merged

Add apis for machine-class #488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
64b6470
Add APIs for MachineClass
hardikdr Oct 9, 2018
a68fc1e
Autogenerated files
hardikdr Oct 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions config/crds/cluster_v1alpha1_cluster.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@ spec:
value:
type: object
valueFrom:
properties:
machineClass:
properties:
provider:
type: string
type: object
type: object
type: object
required:
Expand Down
6 changes: 6 additions & 0 deletions config/crds/cluster_v1alpha1_machine.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ spec:
value:
type: object
valueFrom:
properties:
machineClass:
properties:
provider:
type: string
type: object
type: object
type: object
taints:
Expand Down
33 changes: 33 additions & 0 deletions config/crds/cluster_v1alpha1_machineclass.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
creationTimestamp: null
labels:
controller-tools.k8s.io: "1.0"
name: machineclasses.cluster.k8s.io
spec:
group: cluster.k8s.io
names:
kind: MachineClass
plural: machineclasses
scope: Namespaced
validation:
openAPIV3Schema:
properties:
apiVersion:
type: string
kind:
type: string
metadata:
type: object
providerConfig:
type: object
required:
- providerConfig
version: v1alpha1
status:
acceptedNames:
kind: ""
plural: ""
conditions: []
storedVersions: []
6 changes: 6 additions & 0 deletions config/crds/cluster_v1alpha1_machinedeployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,12 @@ spec:
value:
type: object
valueFrom:
properties:
machineClass:
properties:
provider:
type: string
type: object
type: object
type: object
taints:
Expand Down
6 changes: 6 additions & 0 deletions config/crds/cluster_v1alpha1_machineset.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ spec:
value:
type: object
valueFrom:
properties:
machineClass:
properties:
provider:
type: string
type: object
type: object
type: object
taints:
Expand Down
19 changes: 17 additions & 2 deletions pkg/apis/cluster/v1alpha1/common_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ limitations under the License.

package v1alpha1

import "k8s.io/apimachinery/pkg/runtime"
import (
corev1 "k8s.io/api/core/v1"
runtime "k8s.io/apimachinery/pkg/runtime"
)

// ProviderConfig defines the configuration to use during node creation.
type ProviderConfig struct {
Expand All @@ -39,5 +42,17 @@ type ProviderConfig struct {
// ProviderConfigSource represents a source for the provider-specific
// resource configuration.
type ProviderConfigSource struct {
Copy link
Member Author

@hardikdr hardikdr Aug 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

mvladev: https://github.com/kubernetes/kube-deploy/pull/659/files#r177866614

roberthbailey: I don't think it should be necessary to specify the capacity / allocatable on a single machine if you inline the provider config. I think of the class is a separate way to bring the provider specific data, not something inherent to the spec of the machine.

Copy link

Choose a reason for hiding this comment

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

/cc @mvladev - additional thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@roberthbailey does that mean machine class is just a collection of provider specific configuration?
Advantages of machine classes:

  • less configuration lines in the machine objects by keeping common bits in a machine class object

Any other?

Copy link
Member Author

Choose a reason for hiding this comment

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

less configuration lines in the machine objects by keeping common bits in a machine class object

  • more precisely, externalize the providerConfig to separate objects.
  • as discussed in wg-call, it could also be used for defaulting/versioning via apiserver-webhooks, which could be hard to achieve in only inlined.

// TODO(roberthbailey): Fill these in later
// The machine class from which the provider config should be sourced.
// +optional
MachineClass *MachineClassRef `json:"machineClass,omitempty"`
}

// MachineClassRef is a reference to the MachineClass object. Controllers should find the right MachineClass using this reference.
type MachineClassRef 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.

mvladev: What about namespace?

roberthbailey: Storage classes aren't namespaced, and I used that same pattern here. But it's worth discussing whether they should be part of a namespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have entertained the namespaces- should be helpful especially keeping in mind the sensitivity of the data in machine-classes and our approach of isolating the clusters based on namespaces.

Copy link

Choose a reason for hiding this comment

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

StorageClass are basically templates. Here, we're using namespace for isolating actual clusters. Do we need to also isolate machine templates? I don't see a need for namespace here.

Copy link
Member Author

Choose a reason for hiding this comment

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

As mentioned, machine-classes going forward would be expected to have specific details towards machines and in-turn clusters, and also sensitive information of underlying infra. Separating machine-classes via namespaces will help to avoid unintended consumers of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

tend to agree with @hardikdr on keeping machine class as namespaced object.

// +optional
*corev1.ObjectReference `json:",inline"`

// Provider is the name of the cloud-provider which MachineClass is intended for.
// +optional
Provider string `json:"provider,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that this is needed as the MachineClass should contain all the information for that specific machine.

Copy link

Choose a reason for hiding this comment

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

@mvladev - where does the machine class contain that info? Right now it's contained in this string.

Copy link

@sflxn sflxn Sep 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I assume Provider is similar to StorageClass' "provisioner"? If so, shouldn't we also have a name field for the MachineClass? Or is that already built in because of corev1.ObjectReference?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, ObjectReference should provide us with the name field in it.

}
64 changes: 64 additions & 0 deletions pkg/apis/cluster/v1alpha1/machineclass_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
Copyright 2018 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package v1alpha1

import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
)

// +genclient
// +genclient:noStatus
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

// MachineClass can be used to templatize and re-use provider configuration
// across multiple Machines / MachineSets / MachineDeployments.
// +k8s:openapi-gen=true
// +resource:path=machineclasses
type MachineClass struct {
Copy link

Choose a reason for hiding this comment

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

If we leave out both allocatable and capacity, then machine class == provider config. I think it's important that we address @mvladev's comment above about whether we should have both of those fields on all machines (via an inline class) or only available to the autoscaler on machines that were created via a reference to a machine class.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I believe these decisions would be easier to take once requirements from cluster-autosclaer requirements are clearer. Shall we evolve the APIs for such fields as and when needed?
See: Initial work has started here kubernetes/community#2653

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if allocatable and capacity is something which should be put in the status of a machine. How can one define allocatable?
At the moment this is calculated by summing kube-reserved, system-reserved and eviction-thresholds https://kubernetes.io/docs/tasks/administer-cluster/reserve-compute-resources/#node-allocatable

Copy link

Choose a reason for hiding this comment

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

We are going to chat with some folks that work on the autoscaler tomorrow; maybe that will help clarify what is needed here.

Copy link
Member

@enxebre enxebre Sep 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Code on how allocatable is calculated is here https://github.com/kubernetes/kubernetes/blob/426ef9d349bb3a277c3e8826fb772b6bdb008382/pkg/kubelet/cm/node_container_manager.go#L174:33
We could assume a reasonable margin so allocatable is set based on the given capacity when creating a new machineClass. This would be used for the case of scaling out from zero, otherwise the info from running machines/nodes would be used.

Also should taints belongs to the machineClass?

Copy link

Choose a reason for hiding this comment

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

It seems strange to have allocatable and capacity part of machineclass. Is this defining how many pods we're going to allow to run on this machine? How can we know that without knowing the size of OS, and additional software stack? Shouldn't we let the control plane calculate that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, these fields are expected to be available only after certain calculation, but autoscaler probably would be asking for it from a machine-api stack. We are in talks with autoscaler folks and try to decide right place/design, and hence I have intentionally kept those fields out for the first cut.

Copy link
Member Author

@hardikdr hardikdr Sep 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@enxebre I am not sure about the specific requirements around taints, but I would expect Taints to be available rather at MachineObjects than at class. Class should be seen more for representation of set of machines under MachineDeployment/Set, where subset of them could or could not have taints, which could be best identified by taints on specific MachineObjects.

metav1.TypeMeta `json:",inline"`
// +optional
metav1.ObjectMeta `json:"metadata,omitempty"`

// The total capacity available on this machine type (cpu/memory/disk).
//
// WARNING: It is up to the creator of the MachineClass to ensure that
// this field is consistent with the underlying machine that will
// be provisioned when this class is used, to inform higher level
// automation (e.g. the cluster autoscaler).
// TODO(hardikdr) Add allocatable field once requirements are clear from autoscaler-clusterapi // integration topic.
// Capacity corev1.ResourceList `json:"capacity"`

// How much capacity is actually allocatable on this machine.
// Must be equal to or less than the capacity, and when less
// indicates the resources reserved for system overhead.
//
// WARNING: It is up to the creator of the MachineClass to ensure that
// this field is consistent with the underlying machine that will
// be provisioned when this class is used, to inform higher level
// automation (e.g. the cluster autoscaler).
// TODO(hardikdr) Add allocatable field once requirements are clear from autoscaler-clusterapi // integration topic.
// Allocatable corev1.ResourceList `json:"allocatable"`

// Provider-specific configuration to use during node creation.
ProviderConfig runtime.RawExtension `json:"providerConfig"`

// TODO: should this use an api.ObjectReference to a 'MachineTemplate' instead?
// A link to the MachineTemplate that will be used to create provider
// specific configuration for Machines of this class.
// MachineTemplate corev1.ObjectReference `json:machineTemplate`
Copy link
Member Author

Choose a reason for hiding this comment

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

mladev: Why is this needed?

roberthbailey: I added this comment as an alternate design -- one thing we'd thought about was splitting this data across two objects (class + template) and the template could potentially be used directly by machine in addition to machine class. Not sure if that's valuable or not though, so I put this here to foster discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for keeping capacity and allocatable as MahcineStatus fields. cluster-api controller would initialize these fields once linking with the kubelet node is established. These are "observed" values and mary vary from machine to machine due to some discovery failure or hw failure etc. Therefore should be updated in MachineStatus whatever is actually observed at kubelet node.

+1 for keeping taints in MachineClassSpec. Taints are generally used for scheduling domain isolation and therefore generally applies to a bunch of machines.

}
55 changes: 54 additions & 1 deletion pkg/apis/cluster/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 31 additions & 0 deletions sample/machineclass.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# Sample machine-class object
apiVersion: "cluster.k8s.io/v1alpha1"
kind: MachineClass
metadata:
name: small
namespace: foo
providerConfig:
apiVersion: "gceproviderconfig/v1alpha1"
kind: "GCEProviderConfig"
project: "$GCLOUD_PROJECT"
zone: "us-central1-f"
machineType: "n1-standard-2"
image: "projects/ubuntu-os-cloud/global/images/family/ubuntu-1604-lts"

---

# Sample machine object
apiVersion: cluster.k8s.io/v1alpha1
kind: Machine
metadata:
name: test-machine
namespace: foo
labels:
test-label: test-label
spec:
providerConfig:
valueFrom:
machineClass:
provider: gcp
Copy link

@sflxn sflxn Sep 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I know this is an example, but can provider be defined in the machine class object above? With the way it was defined in the code above, can provider be provided in either machine class or machine objects?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have kept provider at machine-object level, keeping in mind the usecase where we might not want to fetch the complete MachineClass when MachineObject is seen/updated. In the future iterations, we can think if it's needed at any other layers as well to be populated explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hardikdr

the usecase where we might not want to fetch the complete MachineClass when MachineObject is seen/updated.

I am struggling to understand this use case. Who is "we"? autoscaler?
If it is intended that provider should use default provider config for this machine object and should not refer machine class, may be following will make more sense:

spec:
  providerConfig:
    provider: gcp
    valueFrom:
      machineClass:
        name: ""
        namepsace: ""

i.e if machine class is mentioned it will be refereed otherwise should be left blank for default.

Copy link
Member Author

Choose a reason for hiding this comment

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

Inline providerConfig will still work as it is currently, classes will only be a parallel mechanism for users to inject the providerConfig via reference. Above comment was explicitly for provider field at MachineObject layer, and not the existence of classes as such :)

name: small
namespace: foo