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

[Logging] add context info for yunikorn logger #2522

Merged
merged 2 commits into from
Nov 11, 2024

Conversation

win5923
Copy link
Contributor

@win5923 win5923 commented Nov 8, 2024

Why are these changes needed?

Add a helper method to generate a logger that includes RayCluster name and namespace. This can reduce the repetitive code of manually adding context of Raycluster for yunikorn logger.

Before:

{"level":"info","ts":"2024-11-07T13:53:28.458Z","logger":"yunikorn","msg":"Updating pod label based on RayCluster labels","sourceKey":"yunikorn.apache.org/app-id","targetKey":"applicationId","value":"test-yunikorn-0"}
{"level":"info","ts":"2024-11-07T13:53:28.458Z","logger":"yunikorn","msg":"Updating pod label based on RayCluster labels","sourceKey":"yunikorn.apache.org/queue","targetKey":"queue","value":"root.test"}
{"level":"info","ts":"2024-11-07T13:53:28.458Z","logger":"yunikorn","msg":"add task groups info to pod's annotation","key":"yunikorn.apache.org/task-groups","value":"[{\"minResource\":{\"cpu\":\"1\",\"memory\":\"2Gi\"},\"name\":\"headgroup\",\"minMember\":1},{\"minResource\":{\"cpu\":\"1\",\"memory\":\"1Gi\"},\"name\":\"worker\",\"minMember\":2}]","numOfTaskGroups":2}
{"level":"info","ts":"2024-11-07T13:53:28.458Z","logger":"yunikorn","msg":"Gang Scheduling enabled for RayCluster","RayCluster":"test-yunikorn-0","Namespace":"default"}

After:

{"level":"info","ts":"2024-11-08T11:45:03.285Z","logger":"yunikorn","msg":"Updating pod label based on RayCluster labels","rayCluster":"test-yunikorn-0","namespace":"default","sourceKey":"yunikorn.apache.org/app-id","targetKey":"applicationId","value":"test-yunikorn-0"}
{"level":"info","ts":"2024-11-08T11:45:03.285Z","logger":"yunikorn","msg":"Updating pod label based on RayCluster labels","rayCluster":"test-yunikorn-0","namespace":"default","sourceKey":"yunikorn.apache.org/queue","targetKey":"queue","value":"root.test"}
{"level":"info","ts":"2024-11-08T11:45:03.287Z","logger":"yunikorn","msg":"add task groups info to pod's annotation","rayCluster":"test-yunikorn-0","namespace":"default","key":"yunikorn.apache.org/task-groups","value":"[{\"minResource\":{\"cpu\":\"1\",\"memory\":\"2Gi\"},\"name\":\"headgroup\",\"minMember\":1},{\"minResource\":{\"cpu\":\"1\",\"memory\":\"1Gi\"},\"name\":\"worker\",\"minMember\":2}]","numOfTaskGroups":2}
{"level":"info","ts":"2024-11-08T11:45:03.287Z","logger":"yunikorn","msg":"Gang Scheduling enabled for RayCluster","rayCluster":"test-yunikorn-0","namespace":"default"}

Related issue number

Closes #2408

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

@win5923
Copy link
Contributor Author

win5923 commented Nov 8, 2024

cc @kevin85421

@kevin85421 kevin85421 self-assigned this Nov 8, 2024
Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

Is it possible to pass ctx from RayCluster reconciler to YuniKorn's constructor instead?

func (yf *YuniKornSchedulerFactory) New(_ *rest.Config) (schedulerinterface.BatchScheduler, error) {
return &YuniKornScheduler{
log: logf.Log.WithName(SchedulerName),
}, nil
}

The code should look like:

func (yf *YuniKornSchedulerFactory) New(ctx, _ *rest.Config) (schedulerinterface.BatchScheduler, error) {
        logger := ctrl.LoggerFrom(ctx).WithName(SchedulerName)
	return &YuniKornScheduler{
		log: logger
	}, nil
}

@win5923 win5923 marked this pull request as draft November 8, 2024 17:25
@kevin85421
Copy link
Member

Can you rebase with the master branch? I just fixed a CI issue.

@win5923 win5923 force-pushed the feat/YonikornLogger branch 2 times, most recently from f23ae57 to 95c3830 Compare November 9, 2024 04:31
@win5923
Copy link
Contributor Author

win5923 commented Nov 9, 2024

Hi @kevin85421, I tried the method you suggested, but upon checking the logs, there is no RayCluster context.

{"level":"info","ts":"2024-11-09T05:19:49.087Z","logger":"yunikorn","msg":"Gang Scheduling enabled for RayCluster"}

Or should I use logger := ctrl.LoggerFrom(ctx) to get the context for each function like #1945?

@kevin85421
Copy link
Member

Hi @kevin85421, I tried the method you suggested, but upon checking the logs, there is no RayCluster context.

{"level":"info","ts":"2024-11-09T05:19:49.087Z","logger":"yunikorn","msg":"Gang Scheduling enabled for RayCluster"}

Or should I use logger := ctrl.LoggerFrom(ctx) to get the context for each function like #1945?

Oh, I found that NewReconciler is called before SetupWithManager (where the logger is set up).

@kevin85421
Copy link
Member

I read controller-runtime's source code, and I found it reconstruct the logger inside ctx for each reconciliation.

https://github.com/kubernetes-sigs/controller-runtime/blob/63a38006374f1506d75b9eecb65c3f7af1151de5/pkg/internal/controller/controller.go#L309

I think persisting and reusing the logger may cause issues. Maybe we need to pass ctx to the scheduler's functions, and then reconstruct a new logger by:

logger := ctrl.LoggerFrom(ctx).WithName(...)

@win5923 win5923 marked this pull request as ready for review November 9, 2024 12:26
@win5923
Copy link
Contributor Author

win5923 commented Nov 9, 2024

I read controller-runtime's source code, and I found it reconstruct the logger inside ctx for each reconciliation.

https://github.com/kubernetes-sigs/controller-runtime/blob/63a38006374f1506d75b9eecb65c3f7af1151de5/pkg/internal/controller/controller.go#L309

I think persisting and reusing the logger may cause issues. Maybe we need to pass ctx to the scheduler's functions, and then reconstruct a new logger by:

logger := ctrl.LoggerFrom(ctx).WithName(...)

The log are as follows:

{"level":"info","ts":"2024-11-09T12:24:21.107Z","logger":"controllers.RayCluster.yunikorn","msg":"Updating pod label based on RayCluster labels","RayCluster":{"name":"test-yunikorn-0","namespace":"default"},"reconcileID":"48b2bcf9-8371-4ea8-b6c2-448e9006b771","sourceKey":"yunikorn.apache.org/app-id","targetKey":"applicationId","value":"test-yunikorn-0"}
{"level":"info","ts":"2024-11-09T12:24:21.107Z","logger":"controllers.RayCluster.yunikorn","msg":"Updating pod label based on RayCluster labels","RayCluster":{"name":"test-yunikorn-0","namespace":"default"},"reconcileID":"48b2bcf9-8371-4ea8-b6c2-448e9006b771","sourceKey":"yunikorn.apache.org/queue","targetKey":"queue","value":"root.test"}
{"level":"info","ts":"2024-11-09T12:24:21.107Z","logger":"controllers.RayCluster.yunikorn","msg":"add task groups info to pod's annotation","RayCluster":{"name":"test-yunikorn-0","namespace":"default"},"reconcileID":"48b2bcf9-8371-4ea8-b6c2-448e9006b771","key":"yunikorn.apache.org/task-groups","value":"[{\"minResource\":{\"cpu\":\"1\",\"memory\":\"2Gi\"},\"name\":\"headgroup\",\"minMember\":1},{\"minResource\":{\"cpu\":\"1\",\"memory\":\"1Gi\"},\"name\":\"worker\",\"minMember\":2}]","numOfTaskGroups":2}
{"level":"info","ts":"2024-11-09T12:24:21.107Z","logger":"controllers.RayCluster.yunikorn","msg":"Gang Scheduling enabled for RayCluster","RayCluster":{"name":"test-yunikorn-0","namespace":"default"},"reconcileID":"48b2bcf9-8371-4ea8-b6c2-448e9006b771"}

@kevin85421 kevin85421 merged commit 23b08e0 into ray-project:master Nov 11, 2024
25 checks passed
@win5923 win5923 deleted the feat/YonikornLogger branch November 11, 2024 17:40
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.

[Bug] Yunikorn logger doesn't add context info
2 participants