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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port forwarding error immediately after installing the GitOps Dashboard not on the same run when installing Flux #2497

Merged
merged 1 commit into from
Jul 27, 2022

Conversation

opudrovs
Copy link
Contributor

@opudrovs opudrovs commented Jul 26, 2022

Closes #2489

It looks like the bug was caused by a big "typo": when moving the "wait for dashboard ready"code to a separate file, I copied only the wait for reconciliation code, twice. 馃う

So, I've added code to wait for the dashboard reconciliation and pod running properly. It is working as expected now.

Questions:

  1. Is it OK to use the pod and wait for its status == running to wait for dashboard ready?

I also have some old code which seems to work for our purpose, but it uses a HelmRelease:

// wait for dashboard to be ready
if err := wait.Poll(interval, timeout, func() (bool, error) {
	dashboard := &helmv2.HelmRelease{}
	if err := kubeClient.Get(context.Background(), types.NamespacedName{
		Namespace: namespace,
		Name:      helmReleaseName,
	}, dashboard); err != nil {
		return false, err
	}

	return apimeta.IsStatusConditionPresentAndEqual(dashboard.Status.Conditions, meta.ReadyCondition, metav1.ConditionTrue), nil
}); err != nil {
	return err
}

But I think it's better to use the pod instead of the HelmRelease, correct?

  1. Is it OK to add getting the pod the way I did with GetPodFromSpecMap? Because I don't actually need the whole specmap.

  2. Also added getting any type of pods, including non-running pods with GetPodFromSpecMap.

@chanwit, @ozamosi are you OK with that or do you know of a better way? Would it be better to wait for the deployment object ready instead? If yes, how can I get it?

  1. I tried just directly getting the pod with kubeClient by name, but its name looks like that ww-gitops-weave-gitops-96b7c495f-vsp8c and names without this suffix do not work.

So, if you know how to get the pod directly with kubeClient based on the name without the suffix, I would not need to use GetPodFromSpecMap.

@opudrovs opudrovs added bug Something isn't working team/denim labels Jul 26, 2022
@opudrovs opudrovs requested review from chanwit and ozamosi July 26, 2022 22:10
@opudrovs opudrovs marked this pull request as ready for review July 26, 2022 22:11
@opudrovs opudrovs changed the title Fix waiting for when the dashboard is ready. Port forwarding error immediately after installing the GitOps Dashboard not on the same run when installing Flux Jul 26, 2022
@chanwit
Copy link
Member

chanwit commented Jul 27, 2022

Thank you @opudrovs!

Pods created by a deployment usually have suffixes.
The best way, which was already implemented in GetPodFromSpecMap, to obtain pods was to retrieve them via the app's labels. If GetPodFromSpecMap worked for you, I think it's ok.

@opudrovs
Copy link
Contributor Author

@chanwit ok, thank you! I will check if I can simplify the code and retrieve the pod directly with the app labels.

@opudrovs opudrovs merged commit 7b9b7be into main Jul 27, 2022
@opudrovs opudrovs deleted the 2489-gitops-run-port-forwarding-error branch July 27, 2022 12:03
return false, err
}

return dashboard.Status.GetLastHandledReconcileRequest() == sourceRequestedAt, nil
return dashboard.Status.Phase == corev1.PodRunning, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't mean the pod is running, just that it's entered the phase running. Which is to say, it might be in a crash loop. To know that the pod is actually happy and ready to use, you'd need to check the condition.

I found this page https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle and it's explanation of phase vs conditions helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, very helpful! I will add checking for conditions in the followup PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants