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 support for updateStrategy in Logstash #7466

Merged
merged 6 commits into from
Jan 29, 2024

Conversation

kaisecheng
Copy link
Contributor

Logstash adds updateStrategy in CRD which is a StatefulSetUpdateStrategy
The default type is "RollingUpdate"

sample resource

apiVersion: logstash.k8s.elastic.co/v1alpha1
kind: Logstash
metadata:
  name: logstash-sample
spec:
  count: 3
  version: 8.11.3
  pipelines:
    - pipeline.id: main
      pipeline.workers: 2
      config.string: |
        input { exec { command => 'uptime' interval => 10 } } 
        output { 
          stdout {}
        }
  updateStrategy: 
    type: "RollingUpdate"
    rollingUpdate:
      partition: 1
      maxUnavailable: 1

@botelastic botelastic bot added the triage label Jan 15, 2024
@kaisecheng kaisecheng marked this pull request as ready for review January 16, 2024 12:33
@kaisecheng kaisecheng added >enhancement Enhancement of existing functionality :logstash and removed triage labels Jan 16, 2024
Copy link
Member

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

LGTM!

@kaisecheng
Copy link
Contributor Author

@pebrc @thbkrkr @barkbay This is ready for review

Comment on lines 213 to 220
UpdateStrategy: appsv1.StatefulSetUpdateStrategy{
Type: appsv1.RollingUpdateStatefulSetStrategyType,
RollingUpdate: &appsv1.RollingUpdateStatefulSetStrategy{
MaxUnavailable: &intstr.IntOrString{
IntVal: 1,
},
},
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not from this PR but I am a bit confused by this test. Not sure what it actually verifies. It seems it it is just retrieving the Logstash resource again that we just put in as an expected object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It verifies the value of LogstashStatus is up-to-date

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. But for the addition of the UpdateStrategy a test would be more useful that verifies that the UpdateStrategy specified in the Logstash resource is actually applied to the StatefulSet that is being created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test is updated

@pebrc pebrc merged commit a22bf6d into elastic:main Jan 29, 2024
6 checks passed
@thbkrkr thbkrkr changed the title Logstash adds update strategy Add support for updateStrategy in Logstash Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement Enhancement of existing functionality :logstash v2.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants