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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ISSUE #185] Fix pod ready status reflect to cr #186

Merged
merged 1 commit into from
Dec 22, 2023

Conversation

drivebyer
Copy link
Contributor

What is the purpose of the change

#185

Brief changelog

Fix pod ready status reflect to cr.

Verifying this change

Apply:

apiVersion: rocketmq.apache.org/v1alpha1
kind: Broker
metadata:
  name: broker
  namespace: mcamel-system
spec:
  allowRestart: true
  brokerImage: apacherocketmq/rocketmq-broker:4.5.0-alpine-operator-0.3.0
  env:
  - name: BROKER_MEM
    valueFrom:
      configMapKeyRef:
        key: BROKER_MEM
        name: broker-config
  hostPath: /data/rocketmq/broker
  imagePullPolicy: Always
  nameServers: ""
  replicaPerGroup: 1
  resources:
    limits:
      cpu: 500m
      memory: 20Mi          ########### low memory here #############
    requests:
      cpu: 250m
      memory: 10Mi          ########### low memory here ############# 
  scalePodName: broker-0-master-0
  size: 1
  storageMode: EmptyDir
  volumeClaimTemplates:
  - metadata:
      annotations:
        volume.beta.kubernetes.io/storage-class: rocketmq-storage
      name: broker-storage
    spec:
      accessModes:
      - ReadWriteOnce
      resources:
        requests:
          storage: 8Gi
  volumes:
  - configMap:
      items:
      - key: broker-common.conf
        path: broker-common.conf
      name: broker-config
    name: broker-config

Status always empty

Please go through this checklist to help us incorporate your contribution quickly and easily.

Notice: It would be helpful if you could finish the following checklist (the last one is not necessary) before request the community to review your PR.

  • Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Check RBAC rights for Kubernetes roles.
  • Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist.
  • Run make docker-build to build docker image for operator, try your changes from Pod inside your Kubernetes cluster, not just locally. Also provide screenshots to show that the RocketMQ cluster is healthy after the changes.
  • Before committing your changes, remember to run make manifests to make sure the CRD files are updated.
  • Update documentation if necessary.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

@@ -398,6 +402,15 @@ func getBrokerName(broker *rocketmqv1alpha1.Broker, brokerGroupIndex int) string
return broker.Name + "-" + strconv.Itoa(brokerGroupIndex)
}

func isReady(po corev1.Pod) bool {
for _, cond := range po.Status.Conditions {
if cond.Type == corev1.PodReady {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check PodReady status at this stage? The brokers may be just started and initializing.

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. I means we should wait pod to be ready, then execute follow reconcile code(

if broker.Status.Size != 0 && broker.Spec.Size > broker.Status.Size {
// Get the metadata including subscriptionGroup.json and topics.json from scale source pod
k8s, err := tool.NewK8sClient()
if err != nil {
log.Error(err, "Error occurred while getting K8s Client")
}
sourcePodName := broker.Spec.ScalePodName
topicsCommand := getCopyMetadataJsonCommand(cons.TopicJsonDir, sourcePodName, broker.Namespace, k8s)
log.Info("topicsCommand: " + topicsCommand)
subscriptionGroupCommand := getCopyMetadataJsonCommand(cons.SubscriptionGroupJsonDir, sourcePodName, broker.Namespace, k8s)
log.Info("subscriptionGroupCommand: " + subscriptionGroupCommand)
MakeConfigDirCommand := "mkdir -p " + cons.StoreConfigDir
ChmodDirCommand := "chmod a+rw " + cons.StoreConfigDir
cmdContent := MakeConfigDirCommand + " && " + ChmodDirCommand
if topicsCommand != "" {
cmdContent = cmdContent + " && " + topicsCommand
}
if subscriptionGroupCommand != "" {
cmdContent = cmdContent + " && " + subscriptionGroupCommand
}
cmd = []string{"/bin/bash", "-c", cmdContent}
}
// Update status.Size if needed
if broker.Spec.Size != broker.Status.Size {
log.Info("broker.Status.Size = " + strconv.Itoa(broker.Status.Size))
log.Info("broker.Spec.Size = " + strconv.Itoa(broker.Spec.Size))
broker.Status.Size = broker.Spec.Size
err = r.client.Status().Update(context.TODO(), broker)
if err != nil {
reqLogger.Error(err, "Failed to update Broker Size status.")
}
}
// Update status.Nodes if needed
if !reflect.DeepEqual(podNames, broker.Status.Nodes) {
broker.Status.Nodes = podNames
err = r.client.Status().Update(context.TODO(), broker)
if err != nil {
reqLogger.Error(err, "Failed to update Broker Nodes status.")
}
}
//podList := &corev1.PodList{}
//labelSelector := labels.SelectorFromSet(labelsForBroker(broker.Name))
//listOps := &client.ListOptions{
// Namespace: broker.Namespace,
// LabelSelector: labelSelector,
//}
//err = r.client.List(context.TODO(), listOps, podList)
//if err != nil {
// reqLogger.Error(err, "Failed to list pods.", "Broker.Namespace", broker.Namespace, "Broker.Name", broker.Name)
// return reconcile.Result{}, err
//}
//podNames := getPodNames(podList.Items)
//
//// Update status.Nodes if needed
//if !reflect.DeepEqual(podNames, broker.Status.Nodes) {
// broker.Status.Nodes = podNames
// err := r.client.Status().Update(context.TODO(), broker)
// if err != nil {
// reqLogger.Error(err, "Failed to update Broker status.")
// return reconcile.Result{}, err
// }
//}
return reconcile.Result{Requeue: true, RequeueAfter: time.Duration(cons.RequeueIntervalInSecond) * time.Second}, nil
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how do you think about it

@drivebyer drivebyer requested a review from caigy December 5, 2023 10:19
Copy link
Contributor

@caigy caigy left a comment

Choose a reason for hiding this comment

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

LGTM

@caigy caigy merged commit d5e797b into apache:master Dec 22, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants