Skip to content

Additional volumes capability #736

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 10 commits into from
Apr 15, 2020
Merged

Additional volumes capability #736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7b155ad
Allow additional Volumes to be mounted
seuf Dec 9, 2019
e62242b
added TargetContainers option to determine if additional volume need …
seuf Dec 10, 2019
07b90ea
fixed dependencies
seuf Dec 20, 2019
4639ea7
updated manifest additional volume example
seuf Dec 20, 2019
1916b5f
More validation
erthalion Jan 16, 2020
316a047
More documentation / example about additional volumes
seuf Jan 22, 2020
1cf4cc5
Revert go.sum and go.mod from origin/master
seuf Jan 24, 2020
e667d86
Declare addictionalVolume specs in CRDs
seuf Jan 24, 2020
70eee0e
fixed k8sres after rebase
seuf Apr 6, 2020
556ffab
resolv conflict
seuf Apr 6, 2020
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
22 changes: 22 additions & 0 deletions charts/postgres-operator/crds/postgresqls.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,28 @@ spec:
- teamId
- postgresql
properties:
additionalVolumes:
type: array
items:
type: object
required:
- name
- mountPath
- volumeSource
properties:
name:
type: string
mountPath:
type: string
targetContainers:
type: array
nullable: true
items:
type: string
volumeSource:
type: object
subPath:
type: string
allowedSourceRanges:
type: array
nullable: true
Expand Down
12 changes: 12 additions & 0 deletions docs/reference/cluster_manifest.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,18 @@ These parameters are grouped directly under the `spec` key in the manifest.
[the reference schedule format](https://kubernetes.io/docs/tasks/job/automated-tasks-with-cron-jobs/#schedule)
into account. Optional. Default is: "30 00 \* \* \*"

* **additionalVolumes**
List of additional volumes to mount in each container of the statefulset pod.
Each item must contain a `name`, `mountPath`, and `volumeSource` which is a
[kubernetes volumeSource](https://godoc.org/k8s.io/api/core/v1#VolumeSource).
It allows you to mount existing PersistentVolumeClaims, ConfigMaps and Secrets inside the StatefulSet.
Also an `emptyDir` volume can be shared between initContainer and statefulSet.
Additionaly, you can provide a `SubPath` for volume mount (a file in a configMap source volume, for example).
You can also specify in which container the additional Volumes will be mounted with the `targetContainers` array option.
If `targetContainers` is empty, additional volumes will be mounted only in the `postgres` container.
If you set the `all` special item, it will be mounted in all containers (postgres + sidecars).
Else you can set the list of target containers in which the additional volumes will be mounted (eg : postgres, telegraf)

## Postgres parameters

Those parameters are grouped under the `postgresql` top-level key, which is
Expand Down
23 changes: 23 additions & 0 deletions manifests/complete-postgres-manifest.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,29 @@ spec:
volume:
size: 1Gi
# storageClass: my-sc
additionalVolumes:
- name: data
mountPath: /home/postgres/pgdata/partitions
targetContainers:
- postgres
volumeSource:
PersistentVolumeClaim:
claimName: pvc-postgresql-data-partitions
readyOnly: false
- name: conf
mountPath: /etc/telegraf
subPath: telegraf.conf
targetContainers:
- telegraf-sidecar
volumeSource:
configMap:
name: my-config-map
- name: empty
mountPath: /opt/empty
targetContainers:
- all
volumeSource:
emptyDir: {}
numberOfInstances: 2
users: # Application/Robot users
zalando:
Expand Down
22 changes: 22 additions & 0 deletions manifests/postgresql.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,28 @@ spec:
- teamId
- postgresql
properties:
additionalVolumes:
type: array
items:
type: object
required:
- name
- mountPath
- volumeSource
properties:
name:
type: string
mountPath:
type: string
targetContainers:
type: array
nullable: true
items:
type: string
volumeSource:
type: object
subPath:
type: string
allowedSourceRanges:
type: array
nullable: true
Expand Down
31 changes: 31 additions & 0 deletions pkg/apis/acid.zalan.do/v1/crds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,37 @@ var PostgresCRDResourceValidation = apiextv1beta1.CustomResourceValidation{
},
},
},
"additionalVolumes": {
Type: "array",
Items: &apiextv1beta1.JSONSchemaPropsOrArray{
Schema: &apiextv1beta1.JSONSchemaProps{
Type: "object",
Required: []string{"name", "mountPath", "volumeSource"},
Properties: map[string]apiextv1beta1.JSONSchemaProps{
"name": {
Type: "string",
},
"mountPath": {
Type: "string",
},
"targetContainers": {
Type: "array",
Items: &apiextv1beta1.JSONSchemaPropsOrArray{
Schema: &apiextv1beta1.JSONSchemaProps{
Type: "string",
},
},
},
"volumeSource": {
Type: "object",
},
"subPath": {
Type: "string",
},
},
},
},
},
},
},
"status": {
Expand Down
9 changes: 9 additions & 0 deletions pkg/apis/acid.zalan.do/v1/postgresql_type.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ type PostgresSpec struct {
PodAnnotations map[string]string `json:"podAnnotations"`
ServiceAnnotations map[string]string `json:"serviceAnnotations"`
TLS *TLSDescription `json:"tls"`
AdditionalVolumes []AdditionalVolume `json:"additionalVolumes,omitempty"`

// deprecated json tags
InitContainersOld []v1.Container `json:"init_containers,omitempty"`
Expand Down Expand Up @@ -98,6 +99,14 @@ type Volume struct {
SubPath string `json:"subPath,omitempty"`
}

type AdditionalVolume struct {
Name string `json:"name"`
MountPath string `json:"mountPath"`
SubPath string `json:"subPath"`
TargetContainers []string `json:"targetContainers"`
VolumeSource v1.VolumeSource `json:"volume"`
}

// PostgresqlParam describes PostgreSQL version and pairs of configuration parameter name - values.
type PostgresqlParam struct {
PgVersion string `json:"version"`
Expand Down
82 changes: 76 additions & 6 deletions pkg/cluster/k8sres.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ func mountShmVolumeNeeded(opConfig config.Config, spec *acidv1.PostgresSpec) *bo
return opConfig.ShmVolume
}

func generatePodTemplate(
func (c *Cluster) generatePodTemplate(
namespace string,
labels labels.Set,
annotations map[string]string,
Expand All @@ -514,6 +514,7 @@ func generatePodTemplate(
additionalSecretMount string,
additionalSecretMountPath string,
volumes []v1.Volume,
additionalVolumes []acidv1.AdditionalVolume,
Comment on lines 516 to +517
Copy link
Member

Choose a reason for hiding this comment

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

Passing volume[] twice is giving me some headache. The first on was introduced by @zimbatm for the TLS secrets. Maybe that could be mapped to an additionalVolumes too, or it's just renamed tlsVolumes. Anyway, can be done in an extra PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@FxKu -> would this mean that it's either TLS or additionalVolumes?
Our scenario would require both.

Copy link
Member

@FxKu FxKu Apr 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

no I thought both can be mapped into one array. Hm, but maybe it's also better to keep it separated to highlight the special role of TLS secrets. At least, it has it's own field in the manifest.

Copy link
Contributor

@frittentheke frittentheke Apr 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@FxKu if there is a new functionality available fully covering a previously specially handled use case, there should be some sort of deprecation. This here seems to be one of those cases where the "old" functionality is simply extended upon and folks simply can move to the new, more feature rich way.

But this could also be the case for the PR implementing a broader sidecar support (#890). Why having multiple things doing the same thing and creating ever more logic to "merge" them and also having to maintain these increasingly complicated code paths.

Copy link
Contributor

@ReSearchITEng ReSearchITEng Apr 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The two have been merged - see below.
It can be pushed directly: #918
@FxKu and Christian - have a look.
If you want, we can also merge go via @seuf's branch seuf#1 instead - if/once it will be approved by @seuf.
It's same code in both cases.

) (*v1.PodTemplateSpec, error) {

terminateGracePeriodSeconds := terminateGracePeriod
Expand Down Expand Up @@ -553,6 +554,10 @@ func generatePodTemplate(
addSecretVolume(&podSpec, additionalSecretMount, additionalSecretMountPath)
}

if additionalVolumes != nil {
c.addAdditionalVolumes(&podSpec, additionalVolumes)
}

template := v1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Labels: labels,
Expand Down Expand Up @@ -1073,7 +1078,7 @@ func (c *Cluster) generateStatefulSet(spec *acidv1.PostgresSpec) (*appsv1.Statef
annotations := c.generatePodAnnotations(spec)

// generate pod template for the statefulset, based on the spilo container and sidecars
podTemplate, err = generatePodTemplate(
podTemplate, err = c.generatePodTemplate(
c.Namespace,
c.labelsSet(true),
annotations,
Expand All @@ -1093,7 +1098,8 @@ func (c *Cluster) generateStatefulSet(spec *acidv1.PostgresSpec) (*appsv1.Statef
c.OpConfig.AdditionalSecretMount,
c.OpConfig.AdditionalSecretMountPath,
volumes,
)
spec.AdditionalVolumes)

if err != nil {
return nil, fmt.Errorf("could not generate pod template: %v", err)
}
Expand Down Expand Up @@ -1287,6 +1293,69 @@ func addSecretVolume(podSpec *v1.PodSpec, additionalSecretMount string, addition
podSpec.Volumes = volumes
}

func (c *Cluster) addAdditionalVolumes(podSpec *v1.PodSpec,
additionalVolumes []acidv1.AdditionalVolume) {

volumes := podSpec.Volumes
mountPaths := map[string]acidv1.AdditionalVolume{}
for i, v := range additionalVolumes {
if previousVolume, exist := mountPaths[v.MountPath]; exist {
msg := "Volume %+v cannot be mounted to the same path as %+v"
c.logger.Warningf(msg, v, previousVolume)
continue
}

if v.MountPath == constants.PostgresDataMount {
msg := "Cannot mount volume on postgresql data directory, %+v"
c.logger.Warningf(msg, v)
continue
}

if v.TargetContainers == nil {
spiloContainer := podSpec.Containers[0]
additionalVolumes[i].TargetContainers = []string{spiloContainer.Name}
}

for _, target := range v.TargetContainers {
if target == "all" && len(v.TargetContainers) != 1 {
msg := `Target containers could be either "all" or a list
of containers, mixing those is not allowed, %+v`
c.logger.Warningf(msg, v)
continue
}
}

volumes = append(volumes,
v1.Volume{
Name: v.Name,
VolumeSource: v.VolumeSource,
},
)

mountPaths[v.MountPath] = v
}

c.logger.Infof("Mount additional volumes: %+v", additionalVolumes)

for i := range podSpec.Containers {
mounts := podSpec.Containers[i].VolumeMounts
for _, v := range additionalVolumes {
for _, target := range v.TargetContainers {
if podSpec.Containers[i].Name == target || target == "all" {
mounts = append(mounts, v1.VolumeMount{
Name: v.Name,
MountPath: v.MountPath,
SubPath: v.SubPath,
})
}
}
}
podSpec.Containers[i].VolumeMounts = mounts
}

podSpec.Volumes = volumes
}

func generatePersistentVolumeClaimTemplate(volumeSize, volumeStorageClass string) (*v1.PersistentVolumeClaim, error) {

var storageClassName *string
Expand Down Expand Up @@ -1691,7 +1760,7 @@ func (c *Cluster) generateLogicalBackupJob() (*batchv1beta1.CronJob, error) {
annotations := c.generatePodAnnotations(&c.Spec)

// re-use the method that generates DB pod templates
if podTemplate, err = generatePodTemplate(
if podTemplate, err = c.generatePodTemplate(
c.Namespace,
labels,
annotations,
Expand All @@ -1710,8 +1779,9 @@ func (c *Cluster) generateLogicalBackupJob() (*batchv1beta1.CronJob, error) {
"",
c.OpConfig.AdditionalSecretMount,
c.OpConfig.AdditionalSecretMountPath,
nil); err != nil {
return nil, fmt.Errorf("could not generate pod template for logical backup pod: %v", err)
nil,
[]acidv1.AdditionalVolume{}); err != nil {
Copy link

@muff1nman muff1nman Feb 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This should also bring in the same additional volumes here so that the logical backup can benefit from the same customization. This would then make #810 unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting question. I believe it makes sense only if we're going to provide a separate set of "additional" volumes here, probably it's not a good idea to mix volumes for Postgres and for logical backup together. I would suggest we will finish with this PR first, and the open a follow up about doing so for backup, what do you think about this plan?

Copy link

@muff1nman muff1nman Feb 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That would be a departure from the current path considering that additionalSecretMount/Path is mounted on both pods. I don't see any reason to not have these the same.

return nil, fmt.Errorf("could not generate pod template for logical backup pod: %v", err)
}

// overwrite specific params of logical backups pods
Expand Down
Loading