Skip to content
This repository has been archived by the owner on Aug 30, 2023. It is now read-only.

Feature/create deployment status endpoint #203

Merged
merged 34 commits into from
May 23, 2023

Conversation

olis1996
Copy link
Contributor

@olis1996 olis1996 commented Mar 30, 2023

An API endpoint has been implemented to get a deployment's status in kubernetes.
I have a few remaining questions regarding the implementation:

  1. If a user wanted to get a deployment's status by name it would be necessary to call the endpoint something like deployments/by-name/{deploymentName}/status, since quarkus would not be able to differentiate it from deployments/{uuid}/status without the by-name part. Would that implementation be okay or is there a better way that I don't see right now?
  2. Is the io.fabric8.kubernetes.api.model.apps.DeploymentStatus sufficient as "consistent internal representation across different k8s versions"?
  3. Are the tests sufficient?
  4. The testGetDeploymentStatusByValidUuid() test does not pass and I don't understand why. Inside testCreateDeployment() a deployment is created. That deployment's uuid is being used to GET it in the next test testGetDeployment(). But for a reason I don't understand the testGetDeploymentStatusByValidUuid() returns a 404 although manual tests work just fine

@olis1996 olis1996 added enhancement New feature or request api Issues that directly impact user facing apis labels Mar 30, 2023
@olis1996 olis1996 self-assigned this Mar 30, 2023
@olis1996 olis1996 linked an issue Mar 30, 2023 that may be closed by this pull request
@HknLof
Copy link
Contributor

HknLof commented Mar 30, 2023

  1. If a user wanted to get a deployment's status by name it would be necessary to call the endpoint something like deployments/by-name/{deploymentName}/status, since quarkus would not be able to differentiate it from deployments/{uuid}/status without the by-name part. Would that implementation be okay or is there a better way that I don't see right now?

Go with UUID's for now.

  1. Is the io.fabric8.kubernetes.api.model.apps.DeploymentStatus sufficient as "consistent internal representation across different k8s versions"?

No this is not sufficient. This might change with K8s versions, leading to non-stable API's for our users. Fabric8 upgrades lead to model changes if needed by the supported Kubernetes version.

  1. Are the tests sufficient?

An interesting test would be testing the payload. Especially, what is returned, if no replicas are created yet?

  1. The testGetDeploymentStatusByValidUuid() test does not pass and I don't understand why. Inside testCreateDeployment() a deployment is created. That deployment's uuid is being used to GET it in the next test testGetDeployment(). But for a reason I don't understand the testGetDeploymentStatusByValidUuid() returns a 404 although manual tests work just fine.

Not sure what the manual test are from my current reading the following issue is present.
The K8Deployment is never created. https://github.com/DataCater/datacater/blob/main/platform-api/src/main/java/io/datacater/core/deployment/K8Deployment.java#L35
@ChrisRousey might have a quick idea here.

Copy link
Contributor

@ChrisRousey ChrisRousey left a comment

Choose a reason for hiding this comment

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

Good progress 👍🏼 I left some comments with opinionated approaches that should help with the failing test

@olis1996
Copy link
Contributor Author

Not sure what the manual test are from my current reading the following issue is present.

Using the datacater web ui I create a deployment and copy its UUID. In the SwaggerUI I send a GET request to the deployments/{uuid}/status using the copied UUID.

@olis1996 olis1996 marked this pull request as ready for review April 21, 2023 08:24
Copy link
Contributor

@flippingbits flippingbits left a comment

Choose a reason for hiding this comment

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

Thanks for working on this issue :)

In general, this looks good to me. I left two small comments that should be addressed before merging the PR into main.

flippingbits and others added 14 commits April 27, 2023 15:19
Our control plane uses the following default values when creating Apache
Kafka topics:
- num.partitions = 3
- replication.factor = 1

This commit aligns the displayed default values in the frontend with the
ones that are actually used in the control plane.
* feat(ui): add configs to ui initial commit

refs: #181

* feat(ui): change imports error

refs: #181

* feat(ui): changed config icon

refs: #181

* feat(ui): fixed minor bugs when using configs

refs: #181

* feat(ui): added functionality to newConfig for kind and labels

refs: #181

* feat(ui): add logic for deployment configs

refs: #181

* feat(ui): added stream config options to configs

refs: #181

* feat(ui): added stream SerDe options to configs

refs: #181

* feat(ui): added stream Connection and topic config options to configs

resfs: #181

* feat(ui): update config spec on changes to streams

refs: #181

* feat(ui): set config spec before updating

refs: #181

* fat(ui): fix show config bug

refs: #181

* feat(ui): add ground work for update config

refs: #181

* feat(ui): added configSelectors to create deployment and stream

refs: #181

* feast(ui): remove whitespaces

refs: #181

* feat(ui): added configSelectors to editDeployment

refs: #181

* feat(ui): added configSelectors to editStream

refs: #181

* feat(ui): updated editConfig header

refs: #181

* feat(ui): fixed error 500 when editing streams and deployments

refs: #181

* feat(ui): fixed edit config errors

refs: #181

* feat(ui): minor fixes to new functionalities

refs: #181

* feat(ui): fixed spec nesting bug

refs: #181

* chore(ui): Apply npx prettier

* chore: Capitalize kind

* chore: Move labels above kind

* feat(ui/configs): Rework layouts of config forms

* feat(ui/configs): Introduce navbar to forms

* chore(ui/configs): Change wording of subtitle

* feat(ui/configs): Add config selector for deployments

* feat(ui/configs): Add config selector for streams

* fix: fixed bug where replication factor wasn't set

refs: #181

* fix: add config depth matching to avoid overwriting explicit stream configs

refs: #181

* feat(configs): document depth mapping

refs: #181

* feat(configs): add extra checks to depth config mapping

refs: #181

* feat(configs): fix typos

refs: #181

* feat(configs): fix typos

refs: #181

* fix: run pre-commit

* fix(ui): Align default values with backend

* chore(ui/configs): Correctly initialize kind selectbox

---------

Co-authored-by: Stefan Sprenger <stefan@datacater.io>
…#200)

* fix(streams): Avoid merging config into streams when updating streams

When updating a stream object using the endpoint `PUT /streams/:uuid`,
we should not merge referenced config objects into the stream object
before persisting it.

* chore: Clean up comment

* chore(streams): Move deep-copy functionality into Stream class

* chore(streams): Fix typo in function name

* chore(streams): Make pre-commit happy

* chore(streams): Deserialize stream spec before serializing it

* chore(streams): Use copied spec
* feat(ui): added deployment replicas

* feat(ui): parse replica amount as integer

* feat(ui): rnu prettier

* feat(ui): minor changes to deployment replica fields

* fix(ui/deployments): Allow to set replicas field to 0

* fix(ui/deployments): Get pipelines before deployment

When editing a deployment, perform the API call for loading the
pipelines before performing the API call for loading the deployment, to
make sure that the dropdown box always shows the associated pipeline.

If the call to `/api/v1/pipelines` is performed last, the associated
pipeline is not shown in the dropdown box.

* fix(ui/configs): Allow to set replicas field to 0

---------

Co-authored-by: Stefan Sprenger <stefan@datacater.io>
* fix(stream-config-mapping): add config mapping to stream inspect

* fix(stream-config-mapping): add stream config mapping to createDeployment

* fix(stream-config-mapping): map stream config during streamDelete

* fix(stream-config-mapping): use hibernate session as parameter to guarantee same thread

* fix(stream-config-mapping): fix session usage

* fix(stream-config-mapping): add updateEntity to deployment

* fix(stream-config-mapping): update deploymentSpec by value not reference
* feat: prep version bump

* feat: bump mem for pythonrunner to 150M

* feat: bump versions to 2023.2

* fix: bump appversion as well

* feat: postgres manifest for datacater

* feat: upgrade postgres default ns manifest
* feat(ui/deployments): Allow selecting a deployment replica

We support running deployments with multiple replicas.

This commit adds support for selecting a specific replica when viewing a
deployment.

The UI shows the health/metrics/logs of the selected
replica.

By default, replica 1 is selected.

* chore(ui/deployments): Let user know if no replica is available
Add a security policy to our codebase, which tells users:

* the releases that we support with security-related updates
* how to submit security issues.
* fix(platform-api): Update application version

Update the application version to `2023.2` at locations that still
reference to the prior release (`2023.1`).

* fix(platform-api): Update application version for redpanda manifest

---------

Co-authored-by: ChrisRousey <104754971+ChrisRousey@users.noreply.github.com>
HknLof and others added 2 commits April 27, 2023 15:19
* feat(k8-devservice): initial work for devservices

-add dev service config
-remove k8 mock servers

refs: #193

* feat(k8-devservice): removed unneeded dependency

* feat(k8-devservice): add a test for the actual underlying k8 resources

* Revert "Merge branch 'main' into feat/utilize-k8-devservices"

This reverts commit 2064850, reversing
changes made to 6283538.

* Revert "Revert "Merge branch 'main' into feat/utilize-k8-devservices""

This reverts commit 51a87c3.

* fix: fix incorrect merge from main

* fix: fix failing tests after merging from main
@sonarcloud
Copy link

sonarcloud bot commented Apr 27, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

39.1% 39.1% Coverage
0.0% 0.0% Duplication

@olis1996
Copy link
Contributor Author

I removed the commented out code + TODO comments, implemented tests for the status endpoint and added default values for the deployment status as follows:

additionalProperties = {},
availableReplicas = 0,
collisionCount = 0,
conditions = [],
observedGeneration = 0,
readyReplicas = 0,
replicas = 0,
unavailableReplicas = 0,
updatedReplicas = 0

Please let me know what you think, especially about the implementation of the default values with optionals. Is that a good approach?

@sonarcloud
Copy link

sonarcloud bot commented May 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

85.4% 85.4% Coverage
0.0% 0.0% Duplication

github had a timeout due to downtime
Copy link
Contributor

@ChrisRousey ChrisRousey left a comment

Choose a reason for hiding this comment

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

Nice work on ths PR, thank you. I do have two small comments though that i didn't see yesterday since the PR was a bit cluttered before you updated from main.

@HknLof
Copy link
Contributor

HknLof commented May 12, 2023

Thanks, the PR looks good. The last bit would be to have an DataCater internal representation for DeploymentCondition. I love the exhaustive tests looking into the response

Copy link
Contributor

@flippingbits flippingbits left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the great work! I like it a lot. I left one comment, which you might want to address before merging the PR.

HknLof
HknLof previously approved these changes May 23, 2023
Copy link
Contributor

@HknLof HknLof left a comment

Choose a reason for hiding this comment

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

Thanks for bringing this over the finish line!

ChrisRousey
ChrisRousey previously approved these changes May 23, 2023
flippingbits
flippingbits previously approved these changes May 23, 2023
@olis1996 olis1996 dismissed stale reviews from flippingbits, ChrisRousey, and HknLof via 1aac0fc May 23, 2023 08:45
@flippingbits flippingbits self-requested a review May 23, 2023 08:48
Copy link
Contributor

@flippingbits flippingbits left a comment

Choose a reason for hiding this comment

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

Thanks for resolving the minor issue, let's merge this into main! :)

@olis1996 olis1996 merged commit 7e2224f into main May 23, 2023
@sonarcloud
Copy link

sonarcloud bot commented May 23, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

86.4% 86.4% Coverage
0.0% 0.0% Duplication

@flippingbits flippingbits deleted the feature/create-deployment-status-endpoint branch May 25, 2023 07:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api Issues that directly impact user facing apis enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create new endpoint that provides the underlying Kubernetes Deployment
4 participants