Skip to content

Commit

Permalink
fixed missing support for edit of service (custom values, update/repl…
Browse files Browse the repository at this point in the history 
…ace) (#2522)

* fixes in semi-related parts of the system
- removed redundant check
- added documentation of Update/Replace for configurations.
- fixed comment typo

* chore: refactored code (app restart after change) common to config update/replace into singular function.
note: this will be used by service update/replace too.

* chore: moved service structures to separate file

* added update/replace models for services, analogous to configs.
further reordered the types to separate catalog from service.

* refactor create api endpoint, backend somewhat
- moved post hook into adressable function, leaving only the closure
- moved deploy code into separate function for use by coming update/replace

* backend for update/replace service data

* api endpoints for service update/replace

* place display of change instructions in separate function

* service update command, logic, client backend

* tests, service update command

* switch to --set/--unset, deprecate --remove (but process it anyway)
  • Loading branch information
@andreas-kupries
andreas-kupries committed Sep 1, 2023
1 parent 636dbd7 commit f3b6abd
Show file tree
Hide file tree
Showing 22 changed files with 1,100 additions and 262 deletions.
2 changes: 1 addition & 1 deletion acceptance/configurations_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ var _ = Describe("Configurations", LConfiguration, func() {
// edit the configuration ...

out, err := env.Epinio("", "configuration", "update", configurationName1,
"--remove", "username",
"--unset", "username",
"--set", "user=ci/cd",
)
Expect(err).ToNot(HaveOccurred(), out)
Expand Down
88 changes: 88 additions & 0 deletions acceptance/services_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1100,4 +1100,92 @@ var _ = Describe("Services", LService, func() {
})
})
})

Describe("service update", func() {
var appName string
var service string

BeforeEach(func() {
settings, err := env.GetSettingsFrom(testenv.EpinioYAML())
Expect(err).ToNot(HaveOccurred())

appName = catalog.NewAppName()
service = catalog.NewServiceName()
containerImageURL := "splatform/sample-app"
serviceHostname := strings.Replace(settings.API, `https://epinio`, service, 1)

env.MakeContainerImageApp(appName, 1, containerImageURL)

By("creating service instance: " + service)
out, err := env.Epinio("", "service", "create", catalogService.Meta.Name, service,
"--chart-value", "ingress.enabled=true",
"--chart-value", "ingress.hostname="+serviceHostname,
"--chart-value", "nesting.here.hello=world",
"--wait")
Expect(err).ToNot(HaveOccurred(), out)

out, err = env.Epinio("", "service", "bind", service, appName)
Expect(err).ToNot(HaveOccurred(), out)

// Wait for the app restart from binding the service to settle
Eventually(func() string {
out, err := env.Epinio("", "app", "list")
Expect(err).ToNot(HaveOccurred(), out)
return out
}, "5m").Should(
HaveATable(
WithHeaders("NAME", "CREATED", "STATUS", "ROUTES", "CONFIGURATIONS", "STATUS DETAILS"),
WithRow(appName, WithDate(), "1/1", appName+".*", ".*", ""),
),
)
})

It("it edits the service, and restarts the app", func() {
By("editing instance: " + service)
out, err := env.Epinio("", "service", "update", service,
"--wait",
"--unset", "ingress.enabled",
"--unset", "ingress.hostname",
"--set", "nesting.here.hello=user",
)
Expect(err).ToNot(HaveOccurred(), out)
Expect(out).To(ContainSubstring("Update Service"))
Expect(out).To(
HaveATable(
WithHeaders("PARAMETER", "OP", "VALUE"),
WithRow("ingress.enabled", "remove", ""),
WithRow("ingress.hostname", "remove", ""),
WithRow("nesting.here.hello", "add\\/change", "user"),
),
)
Expect(out).To(ContainSubstring("Service Changes Saved"))

// Confirm the changes ...
By("checking instance: " + service)
out, err = env.Epinio("", "service", "show", service)
Expect(err).ToNot(HaveOccurred(), out)

Expect(out).To(ContainSubstring("Showing Service"))
Expect(out).To(
HaveATable(
WithHeaders("KEY", "VALUE"),
WithRow("nesting.here.hello", "user"),
),
)
Expect(out).ToNot(ContainSubstring("ingress"))

// Wait for app to resettle ...
By("waiting for app to resettle: " + appName)
Eventually(func() string {
out, err := env.Epinio("", "app", "list")
Expect(err).ToNot(HaveOccurred(), out)
return out
}, "5m").Should(
HaveATable(
WithHeaders("NAME", "CREATED", "STATUS", "ROUTES", "CONFIGURATIONS", "STATUS DETAILS"),
WithRow(appName, WithDate(), "1/1", appName+".*", ".*", ""),
),
)
})
})
})
113 changes: 113 additions & 0 deletions docs/references/api/swagger.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1663,6 +1663,39 @@
}
}
},
"put": {
"description": "Replace the named `Service` in the `Namespace` as per the instructions in the body",
"tags": [
"service"
],
"operationId": "ServiceReplace",
"parameters": [
{
"type": "string",
"name": "Namespace",
"in": "path",
"required": true
},
{
"type": "string",
"name": "Service",
"in": "path",
"required": true
},
{
"name": "Body",
"in": "body",
"schema": {
"$ref": "#/definitions/ServiceReplaceRequest"
}
}
],
"responses": {
"200": {
"$ref": "#/responses/ServiceReplaceResponse"
}
}
},
"delete": {
"tags": [
"service"
Expand Down Expand Up @@ -1695,6 +1728,39 @@
"$ref": "#/responses/ServiceDeleteResponse"
}
}
},
"patch": {
"description": "Update the named `Service` in the `Namespace` as per the instructions in the body",
"tags": [
"service"
],
"operationId": "ServiceUpdate",
"parameters": [
{
"type": "string",
"name": "Namespace",
"in": "path",
"required": true
},
{
"type": "string",
"name": "Service",
"in": "path",
"required": true
},
{
"name": "Body",
"in": "body",
"schema": {
"$ref": "#/definitions/ServiceUpdateRequest"
}
}
],
"responses": {
"200": {
"$ref": "#/responses/ServiceUpdateResponse"
}
}
}
},
"/namespaces/{Namespace}/services/{Service}/bind": {
Expand Down Expand Up @@ -3008,6 +3074,20 @@
},
"x-go-package": "github.com/epinio/epinio/pkg/api/core/v1/models"
},
"ServiceReplaceRequest": {
"description": "ServiceReplaceRequest represents and contains the data needed to\nreplace a service instance (i.e. the custom value keys)",
"type": "object",
"properties": {
"settings": {
"$ref": "#/definitions/ChartValueSettings"
},
"wait": {
"type": "boolean",
"x-go-name": "Wait"
}
},
"x-go-package": "github.com/epinio/epinio/pkg/api/core/v1/models"
},
"ServiceStatus": {
"type": "string",
"x-go-package": "github.com/epinio/epinio/pkg/api/core/v1/models"
Expand All @@ -3022,6 +3102,27 @@
},
"x-go-package": "github.com/epinio/epinio/pkg/api/core/v1/models"
},
"ServiceUpdateRequest": {
"description": "ServiceUpdateRequest represents and contains the data needed to\nupdate a service instance (add/change, and remove custom value keys)",
"type": "object",
"properties": {
"edit": {
"$ref": "#/definitions/ChartValueSettings"
},
"remove": {
"type": "array",
"items": {
"type": "string"
},
"x-go-name": "Remove"
},
"wait": {
"type": "boolean",
"x-go-name": "Wait"
}
},
"x-go-package": "github.com/epinio/epinio/pkg/api/core/v1/models"
},
"StageRef": {
"description": "StageRef references a staging run by ID, currently randomly generated\nfor each POST to the staging endpoint",
"type": "object",
Expand Down Expand Up @@ -3418,6 +3519,12 @@
"ServicePortForwardResponse": {
"description": ""
},
"ServiceReplaceResponse": {
"description": "",
"schema": {
"$ref": "#/definitions/Response"
}
},
"ServiceShowResponse": {
"description": "",
"schema": {
Expand All @@ -3430,6 +3537,12 @@
"$ref": "#/definitions/Response"
}
},
"ServiceUpdateResponse": {
"description": "",
"schema": {
"$ref": "#/definitions/Response"
}
},
"StagingCompleteResponse": {
"description": "",
"schema": {
Expand Down
33 changes: 33 additions & 0 deletions internal/api/v1/application/deploy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
package application

import (
"context"

apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"

Expand Down Expand Up @@ -111,3 +113,34 @@ func Deploy(c *gin.Context) apierror.APIErrors {
})
return nil
}

// Redeploy does not serve a specific handler. It is used by the configuration and service
// update/replace handlers to restart the active set of the named applications. Quiescent
// applications are ignored. This is their means of forcing the applications bound to the changed
// configuration/service to pick up these changes and use them.
func Redeploy(ctx context.Context, cluster *kubernetes.Cluster, namespace string, appNames []string) apierror.APIErrors {
username := requestctx.User(ctx).Username

for _, appName := range appNames {
app, err := application.Lookup(ctx, cluster, namespace, appName)
if err != nil {
return apierror.InternalError(err)
}

// Restart workload, if any
if app.Workload != nil {
// TODO :: This plain restart is different from all other restarts
// (scaling, ev change, bound configurations change) ... The deployment
// actually does not change, at all. A resource the deployment
// references/uses changed, i.e. the configuration. We still have to
// trigger the restart somehow, so that the pod mounting the
// configuration remounts it for the new/changed keys.
_, apiErr := deploy.DeployAppWithRestart(ctx, cluster, app.Meta, username, "")
if apiErr != nil {
return apiErr
}
}
}

return nil
}
35 changes: 6 additions & 29 deletions internal/api/v1/configuration/replace.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,9 @@ package configuration

import (
"github.com/epinio/epinio/helpers/kubernetes"
"github.com/epinio/epinio/internal/api/v1/deploy"
apiapp "github.com/epinio/epinio/internal/api/v1/application"
"github.com/epinio/epinio/internal/api/v1/response"
"github.com/epinio/epinio/internal/application"
"github.com/epinio/epinio/internal/cli/server/requestctx"
"github.com/epinio/epinio/internal/configurations"
apierror "github.com/epinio/epinio/pkg/api/core/v1/errors"
"github.com/epinio/epinio/pkg/api/core/v1/models"
Expand All @@ -40,9 +39,7 @@ func Replace(c *gin.Context) apierror.APIErrors { // nolint:gocyclo // simplific
if err.Error() == "configuration not found" {
return apierror.ConfigurationIsNotKnown(configurationName)
}
if err != nil {
return apierror.InternalError(err)
}
return apierror.InternalError(err)
}

var replaceRequest models.ConfigurationReplaceRequest
Expand All @@ -56,40 +53,20 @@ func Replace(c *gin.Context) apierror.APIErrors { // nolint:gocyclo // simplific
return apierror.InternalError(err)
}

// Perform restart on the candidates which are actually running
if restart {
username := requestctx.User(ctx).Username

// Determine bound apps, as candidates for restart.
appNames, err := application.BoundAppsNamesFor(ctx, cluster, namespace, configurationName)
if err != nil {
return apierror.InternalError(err)
}

for _, appName := range appNames {
app, err := application.Lookup(ctx, cluster, namespace, appName)
if err != nil {
return apierror.InternalError(err)
}

// Restart workload, if any
if app.Workload != nil {
// TODO :: This plain restart is different from all other restarts
// (scaling, ev change, bound configurations change) ... The deployment
// actually does not change, at all. A resource the deployment
// references/uses changed, i.e. the configuration. We still have to
// trigger the restart somehow, so that the pod mounting the
// configuration remounts it for the new/changed keys.
_, apierr := deploy.DeployAppWithRestart(ctx, cluster, app.Meta, username, "")
if apierr != nil {
return apierr
}
}
// Perform restart on the candidates which are actually running
apiErr := apiapp.Redeploy(ctx, cluster, namespace, appNames)
if apiErr != nil {
return apiErr
}
}

// Done

response.OK(c)
return nil
}
Loading

0 comments on commit f3b6abd

Please sign in to comment.