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.

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

Jump to bottom

App version k8s revision #1143

Merged
merged 2 commits into from
May 9, 2023
Merged

App version k8s revision #1143

merged 2 commits into from
May 9, 2023

Conversation

sebastian-alfers
Copy link
Contributor

@sebastian-alfers sebastian-alfers commented May 4, 2023 •
edited

Todo:

  • Unit tests
  • Rebase once next patch of akka 2.8 is out and bumped in master branch (as well as bump to scala 3.2.2)
  • Question: Should extract into own class? Currently the integration test runs off PodDeletionCostDemoApp.scala
  • Docs

sebastian-alfers
sebastian-alfers commented May 4, 2023
View reviewed changes
rolling-update-kubernetes/src/main/scala/akka/rollingupdate/kubernetes/KubernetesApiImpl.scala
case Some(revision) =>
log.info(s"Reading revision from Kubernetes: $revision")
revision
case None => throw new ReadRevisionException(s"Not able to read revision from Kubernetes.")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This log (and the one below) is currently needed to make the integration test work. The it test parses the pod logs with kubectl.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like a reasonable info log regardless 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean, also adding an info log when at this place when loop() is None?

Copy link
Member

Choose a reason for hiding this comment

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

No, the info above, the exception will contain the details and go upstream.

sebastian-alfers
sebastian-alfers commented May 4, 2023
View reviewed changes
build.sbt Outdated Show resolved Hide resolved
patriknw
patriknw reviewed May 4, 2023
View reviewed changes
Copy link
Member

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

looking good

rolling-update-kubernetes/src/main/scala/akka/rollingupdate/kubernetes/AppVersionRevision.scala Outdated Show resolved Hide resolved
rolling-update-kubernetes/src/main/scala/akka/rollingupdate/kubernetes/AppVersionRevision.scala Outdated Show resolved Hide resolved
rolling-update-kubernetes/src/main/scala/akka/rollingupdate/kubernetes/KubernetesApiImpl.scala Outdated Show resolved Hide resolved
rolling-update-kubernetes/src/main/scala/akka/rollingupdate/kubernetes/KubernetesApiImpl.scala Outdated
} yield {
owner
}
}
Copy link
Member

Choose a reason for hiding this comment

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

isn't this the same as

getPod().map(_.metadata.ownerReferences.headOption)

?

Copy link
Member

Choose a reason for hiding this comment

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

Can it have more than one owner reference, should we instead try to find the one with the ReplicaSet naming convention?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the short version is better.

I have to read up under what circumstances a pod can have multiple ownerReferences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we instead try to find the one with the ReplicaSet naming convention?

Yes, I will filter the ownerReferences list to only look at items that are "kind": "ReplicaSet" and take the headOption of that filtered list (which should then only ever contain a single element after the filtering)

rolling-update-kubernetes/src/main/scala/akka/rollingupdate/kubernetes/AppVersionRevision.scala Show resolved Hide resolved
rolling-update-kubernetes/src/main/scala/akka/rollingupdate/kubernetes/AppVersionRevision.scala Outdated Show resolved Hide resolved
rolling-update-kubernetes/src/main/scala/akka/rollingupdate/kubernetes/KubernetesApiImpl.scala Outdated Show resolved Hide resolved
rolling-update-kubernetes/src/main/scala/akka/rollingupdate/kubernetes/KubernetesApiImpl.scala Show resolved Hide resolved
rolling-update-kubernetes/src/main/scala/akka/rollingupdate/kubernetes/KubernetesApiImpl.scala Outdated Show resolved Hide resolved
rolling-update-kubernetes/src/main/scala/akka/rollingupdate/kubernetes/KubernetesApiImpl.scala Outdated Show resolved Hide resolved
sebastian-alfers
sebastian-alfers commented May 4, 2023
View reviewed changes
docs/src/main/paradox/rolling-updates.md
Java
: @@snip [AppVersionRevisionCompileOnly.java](/rolling-update-kubernetes/src/test/java/jdoc/akka/rollingupdate/kubernetes/AppVersionRevisionCompileOnly.java) { #start }

#### Configuration
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This section is totally copied from above, as it needs the the same params.

@sebastian-alfers sebastian-alfers marked this pull request as ready for review May 4, 2023 20:29
@sebastian-alfers sebastian-alfers changed the title WIP: App version k8s revision App version k8s revision May 5, 2023
@sebastian-alfers sebastian-alfers force-pushed the app-version-k8s-revision branch 2 times, most recently from 9f2c9d0 to a9db0ce Compare May 5, 2023 07:31
sebastian-alfers
sebastian-alfers commented May 5, 2023
View reviewed changes
integration-test/kubernetes-api-java/pom.xml
<akka.version>2.7.0</akka.version>
<akka.http.version>10.4.0</akka.http.version>
<akka.version>2.8.1+2-edfacaa3-SNAPSHOT</akka.version>
<akka.http.version>10.5.1</akka.http.version>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johanandren I missed this in your other pr, so bumping it here now.

@sebastian-alfers
Copy link
Contributor Author

@patriknw Should we have a ref to this feature from here https://doc.akka.io/docs/akka/current/additional/rolling-updates.html#cluster-sharding?

johanandren
johanandren reviewed May 5, 2023
View reviewed changes
Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

Lookin good!

rolling-update-kubernetes/src/main/scala/akka/rollingupdate/kubernetes/AppVersionRevision.scala Show resolved Hide resolved
rolling-update-kubernetes/src/main/scala/akka/rollingupdate/kubernetes/KubernetesApiImpl.scala
case Some(revision) =>
log.info(s"Reading revision from Kubernetes: $revision")
revision
case None => throw new ReadRevisionException(s"Not able to read revision from Kubernetes.")
Copy link
Member

Choose a reason for hiding this comment

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

Seems like a reasonable info log regardless 👍

...-update-kubernetes/src/test/scala/akka/rollingupdate/kubernetes/AppVersionRevisionSpec.scala Show resolved Hide resolved
...-update-kubernetes/src/test/scala/akka/rollingupdate/kubernetes/AppVersionRevisionSpec.scala Show resolved Hide resolved
...-update-kubernetes/src/test/scala/akka/rollingupdate/kubernetes/AppVersionRevisionSpec.scala Show resolved Hide resolved
patriknw
patriknw reviewed May 8, 2023
View reviewed changes
Copy link
Member

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

looking good, just something minor...

docs/src/main/paradox/rolling-updates.md Outdated
@@ -180,4 +180,68 @@ roleRef:
apiGroup: rbac.authorization.k8s.io
```

## app-version from ReplicaSet revision
Copy link
Member

Choose a reason for hiding this comment

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

Instead of ReplicaSet I think we can say "Kubernetes Deployment"
Reason is that the user doesn't care about the ReplicaSet (might not even know what that is) but they define and apply the Deployment. That we read from the ReplicaSet is an implementation detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem with that, but the new title app-version from Kubernetes Deployment revision makes it overflow in the left-hand menu.

Copy link
Member

Choose a reason for hiding this comment

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

"app-version from Deployment revision" or "app-version from Deployment" ?

docs/src/main/paradox/rolling-updates.md Outdated

When using Cluster Sharding, it is [recommended](https://doc.akka.io/docs/akka/current/additional/rolling-updates.html#cluster-sharding) for rolling updates. This works well unless you use `kubectl rollout undo` which deploys the previous ReplicaSet configuration which contains the previous value for that config.
Copy link
Member

Choose a reason for hiding this comment

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

for rolling updates. This works well

I think we need something more between those sentences, such as "That means that you have to define an increasing app-version configuration property for each roll out."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the whole config path akka.cluster.app-version

docs/src/main/paradox/rolling-updates.md Outdated

When using Cluster Sharding, it is [recommended](https://doc.akka.io/docs/akka/current/additional/rolling-updates.html#cluster-sharding) for rolling updates. This works well unless you use `kubectl rollout undo` which deploys the previous ReplicaSet configuration which contains the previous value for that config.

To fix this, you can use `AppVersionRevision` to read the current `revision` from the ReplicaSet via the Kubernetes api which always increases, also during a rollback:
Copy link
Member

Choose a reason for hiding this comment

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

maybe spell out the full annotation name and mention that it is a built in annotation in Kubernetes

docs/src/main/paradox/rolling-updates.md Outdated Show resolved Hide resolved
.../rollingupdate-kubernetes/src/main/scala/akka/cluster/bootstrap/PodDeletionCostDemoApp.scala
@@ -23,6 +23,8 @@ object PodDeletionCostDemoApp extends App {

AkkaManagement(system).start()
ClusterBootstrap(system).start()

AppVersionRevision(system).start()
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't be a problem in practise, but theoretically the AppVersionRevision should be started before ClusterBootstrap. Call to Cluster(system).setAppVersionLater should be before joining.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine to me, if that works with the config-based loading of this extension.

johanandren
johanandren approved these changes May 9, 2023
View reviewed changes
Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

LGTM

patriknw
patriknw approved these changes May 9, 2023
View reviewed changes
Copy link
Member

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

LGTM, great work

@patriknw patriknw merged commit 1ea95a2 into akka:main May 9, 2023
13 checks passed
@patriknw patriknw added this to the 1.4.0 milestone May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patriknw patriknw patriknw approved these changes

@johanandren johanandren johanandren approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.4.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sebastian-alfers @patriknw @johanandren