Improve agent sts pvc tests

[pat-s]

Ref #171

@IceBear2k

I was unable to replicate your issue.

helm template charts/woodpecker/charts/agent/ --set persistence.enabled=true renders a valie statefulset for me and doesn't include volumeMounts and volumeClaimTemplates as shown in your screenshot. Can you verify this on your own? Are you setting additional options?

# Source: agent/templates/statefulset.yaml
apiVersion: apps/v1
kind: StatefulSet
metadata:
  name: release-name-agent
  labels:
    helm.sh/chart: agent-0.3.0
    app.kubernetes.io/name: agent
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/version: "2.4.1"
    app.kubernetes.io/managed-by: Helm
spec:
  serviceName: release-name-agent
  replicas: 2
  selector:
    matchLabels:
      app.kubernetes.io/name: agent
      app.kubernetes.io/instance: release-name
  template:
    metadata:
      labels:
        app.kubernetes.io/name: agent
        app.kubernetes.io/instance: release-name
    spec:
      serviceAccountName: release-name-agent
      securityContext:
        {}
      initContainers:
      containers:
        - name: agent
          securityContext:
            {}
          image: "docker.io/woodpeckerci/woodpecker-agent:v2.4.1"
          imagePullPolicy: IfNotPresent
          ports:
            - name: http
              containerPort: 3000
              protocol: TCP
          resources:
            {}
          volumeMounts:
            - name: agent-config
              mountPath: /etc/woodpecker
          env:
            - name: WOODPECKER_BACKEND
              value: "kubernetes"
            - name: WOODPECKER_BACKEND_K8S_NAMESPACE
              value: "woodpecker"
            - name: WOODPECKER_BACKEND_K8S_POD_ANNOTATIONS
              value: ""
            - name: WOODPECKER_BACKEND_K8S_POD_LABELS
              value: ""
            - name: WOODPECKER_BACKEND_K8S_STORAGE_CLASS
              value: ""
            - name: WOODPECKER_BACKEND_K8S_STORAGE_RWX
              value: "true"
            - name: WOODPECKER_BACKEND_K8S_VOLUME_SIZE
              value: "10G"
            - name: WOODPECKER_CONNECT_RETRY_COUNT
              value: "1"
            - name: WOODPECKER_SERVER
              value: "woodpecker-server:9000"
          envFrom:
            - secretRef:
                name: woodpecker-secret
  volumeClaimTemplates:
    - metadata:
        name: agent-config
        namespace:
        annotations:
      spec:
        accessModes:
          - "ReadWriteOnce"
        resources:
          requests:
            storage: 1Gi

In any case, I've also added additional tests in this PR which should verify the behavior described above. The change in statefuleset.yaml is only to prevent rendering an empty volumes: block, but this is unrelated to the claims in #171. The last test in this PR verifies that the mentioned components should not exist in the STS when setting persistence.enabled=false.

From what I can see you are setting woodpecker.agent.persistence.enabled which won't have an effect as it should be agent.persistence.enabled. Maybe that's the issue?

[IceBear2k]

I'm setting agent.persistence.enabled to false - true is the default. So, the test should be: helm template charts/woodpecker/charts/agent/ --set persistence.enabled=false - that results in what I've mentioned in #171

Sorry for the confusion with woodpecker.agent.persistence.enabled vs agent.persistence.enabled - that's because I'm using an umbrella Helm Chart that includes this Chart as a Sub-Chart woodpecker.

[IceBear2k]

The problematic part in my screenshot is the left side, not the right one. So, not the volume mounts are problematic, what's problematic is when there are no volume mounts (persistence.enabled set to false) which results in the following:

# Source: agent/templates/statefulset.yaml
apiVersion: apps/v1
kind: StatefulSet
metadata:
  name: release-name-agent
  labels:
    helm.sh/chart: agent-0.3.0
    app.kubernetes.io/name: agent
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/version: "2.4.1"
    app.kubernetes.io/managed-by: Helm
spec:
  serviceName: release-name-agent
  replicas: 2
  selector:
    matchLabels:
      app.kubernetes.io/name: agent
      app.kubernetes.io/instance: release-name
  template:
    metadata:
      labels:
        app.kubernetes.io/name: agent
        app.kubernetes.io/instance: release-name
    spec:
      serviceAccountName: release-name-agent
      securityContext:
        {}
      initContainers:
      containers:
        - name: agent
          securityContext:
            {}
          image: "docker.io/woodpeckerci/woodpecker-agent:v2.4.1"
          imagePullPolicy: IfNotPresent
          ports:
            - name: http
              containerPort: 3000
              protocol: TCP
          resources:
            {}
          env:
            - name: WOODPECKER_BACKEND
              value: "kubernetes"
            - name: WOODPECKER_BACKEND_K8S_NAMESPACE
              value: "woodpecker"
            - name: WOODPECKER_BACKEND_K8S_POD_ANNOTATIONS
              value: ""
            - name: WOODPECKER_BACKEND_K8S_POD_LABELS
              value: ""
            - name: WOODPECKER_BACKEND_K8S_STORAGE_CLASS
              value: ""
            - name: WOODPECKER_BACKEND_K8S_STORAGE_RWX
              value: "true"
            - name: WOODPECKER_BACKEND_K8S_VOLUME_SIZE
              value: "10G"
            - name: WOODPECKER_CONNECT_RETRY_COUNT
              value: "1"
            - name: WOODPECKER_SERVER
              value: "woodpecker-server:9000"
          envFrom:
            - secretRef:
                name: woodpecker-secret
        - name: data
          emptyDir: {}

While that is valid YAML, have a look at the very last two lines. They make no sense here and will render the Chart invalid.

[pat-s]

So, the test should be: helm template charts/woodpecker/charts/agent/ --set persistence.enabled=false

Sorry, my comment had a typo. It should have been persistence.enabled=false. That is in fact what I checked for and the output above should reflect that.

The tests now cover both cases.

While that is valid YAML, have a look at the very last two lines. They make no sense here and will render the Chart invalid.

Ah sorry 🤦 Now I see it. Thanks for your persistence, I agree. I removed them now.

[pat-s]

@xoxys @anbraten review appreciated :)

[pat-s]

I've made a slight to the logic now again:

• If persistence=false and no extraVolumeMount is given, an emptyDir is mounted (following the initial idea)
• Otherwise only the PVC is mounted

Also adapted the tests.

[xoxys]

I'm still a bit confused. What's the purpose of /data? Is the agent writing anything to this path?

[pat-s]

I have the habit to add an emptyDir scratch volume to all pods by default. But no, in this case, the agent is not writing any data. One can persist the config file to avoid new registrations all the time but this must be done via a PVC.

I'll remove it.