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

Make okteto up wait until original resource is awaken before starting dev container #3368

Merged
merged 5 commits into from Feb 6, 2023
Merged

Make okteto up wait until original resource is awaken before starting dev container #3368

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
601e84a
Changes in the up command to wake the namespace up (if it is sleeping…
ifbyol Jan 31, 2023
18f1e8b
Execute wake call within a goroutine, extracted status label to a con…
ifbyol Feb 1, 2023
be77a7c
If the dev container is an autocreate, we don't need to wait until it…
ifbyol Feb 1, 2023
2fc1d39
Addressed code review comments. Some renames, stop timers properly an…
ifbyol Feb 1, 2023
8894cd6
Merge remote-tracking branch 'origin/master' into ifbyol/up-wait-for-…
ifbyol Feb 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 31 additions & 0 deletions cmd/up/activate.go
View file
Open in desktop
Expand Up @@ -115,6 +115,11 @@ func (up *upContext) activate() error {
lastPodUID = up.Pod.UID
}

if err := up.waitUntilAppIsAwaken(ctx, app); err != nil {
AgustinRamiroDiaz marked this conversation as resolved.
Show resolved Hide resolved
oktetoLog.Infof("error waiting for the original %s to be awaken: %s", app.Kind(), err.Error())
return err
}

if err := up.devMode(ctx, app, create); err != nil {
if oktetoErrors.IsTransient(err) {
return err
Expand Down Expand Up @@ -457,3 +462,29 @@ func getPullingMessage(message, namespace string) string {
toReplace := fmt.Sprintf("%s/%s", registry, namespace)
return strings.Replace(message, toReplace, okteto.DevRegistry, 1)
}

// waitUntilAppIsAwaken waits until the app is awaken checking if the annotation dev.okteto.com/state-before-sleeping is present in the app resource
func (up *upContext) waitUntilAppIsAwaken(ctx context.Context, app apps.App) error {
Copy link
Member Author

Choose a reason for hiding this comment

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

In this case I'm not checking if the context is an Okteto one or not because the resource shouldn't have that annotation in a non Okteto cluster

if _, ok := app.ObjectMeta().Annotations[model.StateBeforeSleepingAnnontation]; !ok {
return nil
}

timeout := 5 * time.Minute
Copy link
Member Author

Choose a reason for hiding this comment

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

@pchico83 I'm setting 5 minutes as the maximum time to wait for a resource to be awaken. Do you think we should increase it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should be using dev.timeout.resources. Default value is 120 seconds. I don't know if that works for you but if we use it it's already configurable in the dev section

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if 120 seconds would be enough. When things starts to be waken up in order it might take a bit more (taking also into account the time for the job to be scheduled)

to := time.NewTicker(timeout)
ifbyol marked this conversation as resolved.
Show resolved Hide resolved
ticker := time.NewTicker(10 * time.Second)
ifbyol marked this conversation as resolved.
Show resolved Hide resolved
ifbyol marked this conversation as resolved.
Show resolved Hide resolved
for {
select {
case <-to.C:
return fmt.Errorf("'%s' '%s' didn't wake up after %s", app.Kind(), app.ObjectMeta().Name, timeout.String())
case <-ticker.C:
if err := app.Refresh(ctx, up.Client); err != nil {
return err
}

// If the app is not sleeping anymore, we are done
if _, ok := app.ObjectMeta().Annotations[model.StateBeforeSleepingAnnontation]; !ok {
return nil
}
ifbyol marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
34 changes: 34 additions & 0 deletions cmd/up/up.go
View file
Open in desktop
Expand Up @@ -36,6 +36,8 @@ import (
"github.com/okteto/okteto/pkg/constants"
"github.com/okteto/okteto/pkg/devenvironment"
"github.com/okteto/okteto/pkg/discovery"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"

"github.com/okteto/okteto/pkg/cmd/pipeline"
"github.com/okteto/okteto/pkg/config"
Expand Down Expand Up @@ -295,6 +297,22 @@ func Up() *cobra.Command {
up.Dev.Autocreate = true
}

// only if the context is an okteto one, we should verify if the namespace has to be woken up
if okteto.Context().IsOkteto {
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if this is the best place to execute this or it could be before, can you check @jLopezbarb ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd leave it here since we need dev.Namespace

Copy link
Contributor

Choose a reason for hiding this comment

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

@ifbyol as you mentioned in the other case where you didn't check if it was an okteto context because the label was not going to be there, could we do the same here? wakeNamespaceIfApplies is checking for the label to be "Sleeping" and if I understand correctly, that could not happen without an okteto context

Copy link
Member Author

Choose a reason for hiding this comment

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

but the code within this if is calling to Okteto API (wakeSpace mutation), so if the context is not an Okteto one, it will fail. We need this check

Copy link
Contributor

Choose a reason for hiding this comment

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

@ifbyol what if are you referring to? What I mean is that wakeNamespaceIfApplies won't call wakeSpace since it will return early in the label check

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but it would slow down things in vanilla clusters as we would need to retrieve the ns to check that. The idea is to execute only that logic if it is needed (in an Okteto context) and execute it within a goroutine to avoid to impact on the okteto up command.

I think it is clearer to throw a goroutine only if the context is an Okteto one (having available that information), that executing a goroutine with the logic to execute the wait within it or not

k8sClient, _, err := okteto.NewK8sClientProvider().Provide(okteto.Context().Cfg)
if err != nil {
return err
}
okClient, err := okteto.NewOktetoClient()
if err != nil {
return err
}
if err := wakeNamespaceIfApplies(ctx, up.Dev.Namespace, k8sClient, okClient); err != nil {
// If there is an error waking up namespace, we don't want to fail the up command
ifbyol marked this conversation as resolved.
Show resolved Hide resolved
oktetoLog.Infof("failed to wake up the namespace: %s", err.Error())
}
}

if err := setBuildEnvVars(ctx, oktetoManifest); err != nil {
return err
}
Expand Down Expand Up @@ -903,3 +921,19 @@ func setBuildEnvVars(ctx context.Context, m *model.Manifest) error {
}
return builder.Build(ctx, buildOptions)
}

// wakeNamespaceIfApplies wakes the namespace if it is sleeping
func wakeNamespaceIfApplies(ctx context.Context, ns string, k8sClient kubernetes.Interface, okClient types.OktetoInterface) error {
n, err := k8sClient.CoreV1().Namespaces().Get(ctx, ns, metav1.GetOptions{})
if err != nil {
return err
}

// If the namespace is not sleeping, do nothing
if n.Labels["space.okteto.com/status"] != "Sleeping" {
ifbyol marked this conversation as resolved.
Show resolved Hide resolved
return nil
}

oktetoLog.Information("Namespace '%s' is sleeping, waking it up...", ns)
return okClient.Namespaces().Wake(ctx, ns)
}
55 changes: 55 additions & 0 deletions cmd/up/up_test.go
View file
Open in desktop
Expand Up @@ -19,10 +19,16 @@ import (
"fmt"
"testing"

"github.com/okteto/okteto/internal/test/client"
oktetoErrors "github.com/okteto/okteto/pkg/errors"
"github.com/okteto/okteto/pkg/model"
"github.com/okteto/okteto/pkg/model/forward"
"github.com/okteto/okteto/pkg/types"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes/fake"
)

func Test_waitUntilExitOrInterrupt(t *testing.T) {
Expand Down Expand Up @@ -343,3 +349,52 @@ func TestCommandAddedToUpOptionsWhenPassedAsFlag(t *testing.T) {
})
}
}

func TestWakeNamespaceIfAppliesWithoutErrors(t *testing.T) {
tests := []struct {
name string
ns v1.Namespace
expectedWakeCalls int
}{
{
name: "wake namespace if it is not sleeping",
ns: v1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Labels: map[string]string{
"space.okteto.com/status": "Active",
},
},
},
expectedWakeCalls: 0,
},
{
name: "wake namespace if it is sleeping",
ns: v1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Labels: map[string]string{
"space.okteto.com/status": "Sleeping",
},
},
},
expectedWakeCalls: 1,
},
}
ctx := context.Background()

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
k8sClient := fake.NewSimpleClientset(&tt.ns)
nsClient := client.NewFakeNamespaceClient([]types.Namespace{}, nil)
oktetoClient := &client.FakeOktetoClient{
Namespace: nsClient,
}

err := wakeNamespaceIfApplies(ctx, tt.ns.Name, k8sClient, oktetoClient)

require.NoError(t, err)
require.Equal(t, tt.expectedWakeCalls, nsClient.WakeCalls)
})
}
}
13 changes: 13 additions & 0 deletions internal/test/client/namespace.go
View file
Open in desktop
Expand Up @@ -22,6 +22,9 @@ import (
type FakeNamespaceClient struct {
namespaces []types.Namespace
err error

// WakeCalls is the number of times Wake was called
WakeCalls int
}

func NewFakeNamespaceClient(ns []types.Namespace, err error) *FakeNamespaceClient {
Expand Down Expand Up @@ -70,3 +73,13 @@ func (*FakeNamespaceClient) SleepNamespace(_ context.Context, _ string) error {
func (*FakeNamespaceClient) DestroyAll(_ context.Context, _ string, _ bool) error {
return nil
}

// Wake wakes up a namespace
func (c *FakeNamespaceClient) Wake(_ context.Context, _ string) error {
if c.err != nil {
return c.err
}

c.WakeCalls++
return nil
}
18 changes: 18 additions & 0 deletions pkg/okteto/namespace.go
View file
Open in desktop
Expand Up @@ -188,3 +188,21 @@ func (c *namespaceClient) DestroyAll(ctx context.Context, namespace string, dest

return nil
}

// Wake wakes a namespace
func (c *namespaceClient) Wake(ctx context.Context, namespace string) error {
var mutation struct {
Space struct {
Id graphql.String
} `graphql:"wakeSpace(space: $space)"`
}
variables := map[string]interface{}{
"space": graphql.String(namespace),
}
err := mutate(ctx, &mutation, variables, c.client)
if err != nil {
return err
}

return nil
}
1 change: 1 addition & 0 deletions pkg/types/interface.go
View file
Open in desktop
Expand Up @@ -40,6 +40,7 @@ type NamespaceInterface interface {
AddMembers(ctx context.Context, namespace string, members []string) error
SleepNamespace(ctx context.Context, namespace string) error
DestroyAll(ctx context.Context, namespace string, destroyVolumes bool) error
Wake(ctx context.Context, namespace string) error
}

// PreviewInterface represents the client that connects to the preview functions
Expand Down