API Changes #123

prydie · 2018-06-04T17:01:57Z

Started work on making the API more idiomatic prior to v0.2 release. There are still a number of changes to make but feedback at this point would be great.

Includes:

Updating dependencies to Kubernetes 1.10.0
Regenerating all generated code incl. rename Mysql -> MySQL

Resolves: #119

prydie · 2018-06-04T17:07:59Z

pkg/apis/mysql/v1alpha1/types.go

+	// SSLSecretRef allows a user to specify custom CA certificate, server certificate
+	// and server key for group replication SSL
+	// +optional
+	SSLSecretRef *corev1.LocalObjectReference `json:"sslSecretRef,omitempty"`


s/SSLSecretRef/RootPasswordSecret/g?

s/SSLSecretRef/SSLSecret/g but i'm guessing that's what you meant ;)

prydie · 2018-06-04T17:08:28Z

pkg/apis/mysql/v1alpha1/types.go

+
+	// ConfigRef allows a user to specify a custom configuration file for MySQL.
+	// +optional
+	ConfigRef *corev1.LocalObjectReference `json:"configRef,omitempty"`


s/ConfigRef/Config/g?

prydie · 2018-06-04T17:10:40Z

pkg/apis/mysql/v1alpha1/types.go

+	// If defined, we use this secret for configuring the MYSQL_ROOT_PASSWORD
+	// If it is not set we generate a secret dynamically
+	// +optional
+	SecretRef *corev1.LocalObjectReference `json:"secretRef,omitempty"`


s/SecretRef/RootPasswordSecret/?

prydie · 2018-06-04T17:11:14Z

pkg/apis/mysql/v1alpha1/types.go

+	// MultiMaster defines the mode of the MySQL cluster. If set to true,
+	// all instances will be R/W. If false (the default), only a single instance
+	// will be R/W and the rest will be R/O.
+	MultiMaster bool `json:"multiMaster,omitempty"`


Add comment and link to why multi-master is probably not what users want.

prydie · 2018-06-04T17:11:46Z

pkg/apis/mysql/v1alpha1/types.go

+type ClusterStatus struct {
+	metav1.TypeMeta   `json:",inline"`
+	metav1.ObjectMeta `json:"metadata"`
+	Phase             ClusterPhase `json:"phase"`


Replace with conditions?

prydie · 2018-06-04T17:12:22Z

pkg/apis/mysql/v1alpha1/types.go

+	metav1.TypeMeta   `json:",inline"`
+	metav1.ObjectMeta `json:"metadata"`
+	Phase             ClusterPhase `json:"phase"`
+	Errors            []string     `json:"errors"`


prydie · 2018-06-04T17:13:03Z

pkg/apis/mysql/v1alpha1/types.go

+// The generation of the backup is configured in the Executor configuration.
+type BackupStorageProvider struct {
+	// Name denotes the type of storage provider that will store and retrieve the backups,
+	// e.g. s3, oci-s3-compat, aws-s3, gce-s3, etc.


s3 only currently

prydie · 2018-06-04T17:13:47Z

pkg/apis/mysql/v1alpha1/types.go

+	Name string `json:"name"`
+	// SecretRef is a reference to the Kubernetes secret containing the configuration for uploading
+	// the backup to authenticated storage.
+	SecretRef *corev1.LocalObjectReference `json:"secretRef,omitempty"`


s/SecretRef/Secret/?

Should this be AuthSecret or similar? Secret on it's own is a bit too generic in k8s world i'd say?

prydie · 2018-06-04T17:14:10Z

pkg/apis/mysql/v1alpha1/types.go

+// BackupStatus captures the current status of a MySQL backup.
+type BackupStatus struct {
+	// Phase is the current life-cycle phase of the Backup.
+	Phase BackupPhase `json:"phase"`


Replace with conditions?

+1

https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#typical-status-properties

Newer API types should use conditions instead.

You should avoid using phase as it's a deprecated pattern in core.

prydie · 2018-06-04T17:14:33Z

pkg/apis/mysql/v1alpha1/types.go

+	Outcome BackupOutcome `json:"outcome"`
+
+	// TimeStarted is the time at which the backup was started.
+	TimeStarted metav1.Time `json:"timeStarted"`


These can be folded into conditions I think?

prydie · 2018-06-04T17:14:55Z

pkg/apis/mysql/v1alpha1/types.go

+// ScheduleStatus captures the current state of a MySQL backup schedule.
+type ScheduleStatus struct {
+	// Phase is the current phase of the MySQL backup schedule.
+	Phase BackupSchedulePhase `json:"phase"`


Replace with conditions?

prydie · 2018-06-04T17:15:23Z

pkg/apis/mysql/v1alpha1/types.go

+type RestoreSpec struct {
+	// ClusterRef is a refeference to the Cluster to which the Restore
+	// belongs.
+	ClusterRef *corev1.LocalObjectReference `json:"clusterRef"`


s/ClusterRef/Cluster/?

prydie · 2018-06-04T17:16:26Z

pkg/apis/mysql/v1alpha1/types.go

+	ClusterRef *corev1.LocalObjectReference `json:"clusterRef"`
+
+	// BackupRef is a reference to the Backup object to be restored.
+	BackupRef *corev1.LocalObjectReference `json:"backupRef"`


s/BackupRef/Backup/?

prydie · 2018-06-04T17:16:40Z

pkg/apis/mysql/v1alpha1/types.go

+// RestoreStatus captures the current status of a MySQL restore.
+type RestoreStatus struct {
+	// Phase is the current life-cycle phase of the Restore.
+	Phase RestorePhase `json:"phase"`


Replace with conditions?

jhorwit2 · 2018-06-06T12:30:03Z

pkg/apis/mysql/v1alpha1/types.go

+
+	// If specified, affinity will define the pod's scheduling constraints
+	// +optional
+	Affinity *corev1.Affinity `json:"affinity,omitempty"`


Affinity breaks PVC/STS. You sure you want this prior to topology aware CSI?

jhorwit2 · 2018-06-06T12:32:16Z

pkg/apis/mysql/v1alpha1/types.go

+
+	// AgentScheduled is the agent hostname to run the backup on.
+	// TODO(apryde): ScheduledAgent (*corev1.LocalObjectReference)?
+	AgentScheduled string `json:"agentscheduled"`


This should be

ScheduledAgent string json:"scheduledAgent"

jhorwit2 · 2018-06-06T12:33:23Z

pkg/apis/mysql/v1alpha1/types.go

+	// Name of the tool performing the backup, e.g. mysqldump.
+	Name string `json:"name"`
+	// Databases are the databases to backup.
+	Databases []string `json:"databases"`


Do you ever see this needing more fields under database? Like the table? If so i'd recommend using a struct with a name field.

jhorwit2 · 2018-06-06T12:33:45Z

pkg/apis/mysql/v1alpha1/types.go

+type BackupStorageProvider struct {
+	// Name denotes the type of storage provider that will store and retrieve the backups.
+	// Currently only supports "S3" denoting a S3 compatiable storage provider.
+	Name string `json:"name"`


Seems like it should be a const.

jhorwit2 · 2018-06-06T12:35:27Z

pkg/apis/mysql/v1alpha1/types.go

+// BackupStatus captures the current status of a MySQL backup.
+type BackupStatus struct {
+	// Phase is the current life-cycle phase of the Backup.
+	Phase BackupPhase `json:"phase"`


+1

https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#typical-status-properties

Newer API types should use conditions instead.

You should avoid using phase as it's a deprecated pattern in core.

jhorwit2 · 2018-06-06T12:36:34Z

pkg/apis/mysql/v1alpha1/types.go

+	Backup *corev1.LocalObjectReference `json:"backup"`
+
+	// AgentScheduled is the agent hostname to run the backup on
+	AgentScheduled string `json:"agentscheduled"`


SheduledAgent looks more correct to me. Also camel case on the json field.

Or even AgentName

Thinking about it Agent is an implementation detail. ScheduledMember maybe?

prydie · 2018-06-07T14:22:45Z

pkg/apis/mysql/v1alpha1/types.go

+	Version string `json:"version"`
+
+	// Replicas defines the number of running MySQL instances in a cluster
+	Replicas int32 `json:"replicas,omitempty"`


Controversially (perhaps) I'd like to change this to Members. Replicas is semantically incorrect for the default and recommended deployment (i.e. single-primary mode). The MySQL documentation discusses cluster "members" and I think we should align with that.

//cc @owainlewis @garthy @jhorwit2

Also Backup/Restore AgentScheduled -> ScheduledMember.

owainlewis · 2018-06-08T12:54:37Z

Gopkg.toml

@@ -49,23 +47,23 @@ required = [

 [[constraint]]
  name = "k8s.io/api"
-  branch = "release-1.9"
+  version = "kubernetes-1.10.0"


Dependencies need approvals.

owainlewis · 2018-06-08T12:55:51Z

docs/user/clusters.md

 metadata:
  name: example-mysql-cluster-with-volume
 spec:
-  replicas: 1
-  secretRef:
+  mem   : 1


Typo => members

owainlewis · 2018-06-11T14:16:57Z

pkg/apis/mysql/v1alpha1/helpers.go

+// TODO (owain) we need to remove this because it's not reasonable for us to maintain a list
+// of all the potential MySQL versions that can be used and in reality, it shouldn't matter
+// too much. The burden of this is not worth the benfit to a user
+var validVersions = []string{


This should be removed based on the changes done for 0.1.1

prydie · 2018-06-12T17:40:47Z

@gianlucaborello I’d be very interested on your thoughts on these API changes if you have a moment to take a look at the updated type definitions.

gianlucaborello · 2018-06-12T18:09:35Z

Took a quick look (without trying it though) and all seems fine to me, overall minor changes from an API perspective. At the moment, as my RFC PRs hint at, my goal is to make sure the cluster membership management logic is as bullet proof as it can be.

Thanks.

owainlewis

Changes approved.

prydie added 4 commits June 4, 2018 16:58

Clean up API (WIP)

23e0549

Bump dependencies to track Kubernetes 1.10

8125cfd

Update generated code

c81ee31

Mysql -> MySQL

3bd99c6

prydie requested a review from jhorwit2 June 4, 2018 17:01

prydie commented Jun 4, 2018

View reviewed changes

prydie added 4 commits June 5, 2018 11:52

More API changes

04d543c

Further refactoring of types pkg

54ea852

Cluster from using phases to conditions

e13d9b6

Fix condition

3841013

jhorwit2 reviewed Jun 6, 2018

View reviewed changes

prydie added 3 commits June 6, 2018 14:24

Use conditions rather than a phase for Backups

a10aa9d

Remove phase from BackupSchedule

619f912

Replace Restore phase with conditions

8ab5c57

prydie added enhancement needs-approval labels Jun 7, 2018

prydie commented Jun 7, 2018

View reviewed changes

prydie added 3 commits June 7, 2018 17:59

Typed backup StorageProviders

8bea60e

Refactor Backup executor API

718bca6

Cluster Replicas -> Members

31358d9

Also Backup/Restore AgentScheduled -> ScheduledMember.

prydie changed the title ~~WIP: API Changes~~ API Changes Jun 8, 2018

prydie added this to the 0.2.0 milestone Jun 8, 2018

owainlewis reviewed Jun 8, 2018

View reviewed changes

owainlewis reviewed Jun 11, 2018

View reviewed changes

Code review comments

5fc0e5b

owainlewis self-assigned this Jun 13, 2018

owainlewis approved these changes Jun 13, 2018

View reviewed changes

owainlewis merged commit 2944c3d into master Jun 13, 2018

This was referenced Jun 13, 2018

Fix tutorial bugs #126

Closed

Fix the documentation to use the correct apiversion #129

Closed

prydie deleted the ap/api-changes branch July 5, 2018 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Changes #123

API Changes #123

prydie commented Jun 4, 2018

prydie Jun 4, 2018 •

edited

Loading

simonlord Jun 5, 2018

prydie Jun 4, 2018

prydie Jun 4, 2018

prydie Jun 4, 2018

prydie Jun 4, 2018

prydie Jun 4, 2018

prydie Jun 4, 2018

prydie Jun 4, 2018

simonlord Jun 5, 2018

prydie Jun 4, 2018

jhorwit2 Jun 6, 2018

prydie Jun 4, 2018

prydie Jun 4, 2018

prydie Jun 4, 2018

prydie Jun 4, 2018

prydie Jun 4, 2018

jhorwit2 Jun 6, 2018

jhorwit2 Jun 6, 2018

jhorwit2 Jun 6, 2018

jhorwit2 Jun 6, 2018

jhorwit2 Jun 6, 2018

jhorwit2 Jun 6, 2018

prydie Jun 7, 2018

prydie Jun 7, 2018

owainlewis Jun 8, 2018

owainlewis Jun 13, 2018

owainlewis Jun 8, 2018

owainlewis Jun 11, 2018 •

edited

Loading

prydie commented Jun 12, 2018

gianlucaborello commented Jun 12, 2018

owainlewis left a comment

API Changes #123

API Changes #123

Conversation

prydie commented Jun 4, 2018

prydie Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owainlewis Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

prydie commented Jun 12, 2018

gianlucaborello commented Jun 12, 2018

owainlewis left a comment

Choose a reason for hiding this comment

prydie Jun 4, 2018 •

edited

Loading

owainlewis Jun 11, 2018 •

edited

Loading