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

LOG-2587: Fix ES functional tests for vector #1454

Closed
wants to merge 1 commit into from
Closed

LOG-2587: Fix ES functional tests for vector #1454

wants to merge 1 commit into from

Conversation

vimalk78
Copy link
Contributor

@vimalk78 vimalk78 commented May 8, 2022

Description

This PR

  • fixes functional framework to make kubernetes_logs start scraping application logs created by functional framework
  • changes generator to add a flag for metrics, so that functional tests do not generate metrics collection.
  • fixes app log data model to conform to functional test app log template
  • refactors functional test to separate the message templates for fluentd and vector
  • enables outputs/elasticsearch tests to run for both fluentd and vector

/cc @jcantrill @cahartma
/assign

/cherry-pick

Links

@openshift-ci openshift-ci bot requested review from cahartma and jcantrill May 8, 2022 06:44
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2022
@vimalk78
Copy link
Contributor Author

vimalk78 commented May 8, 2022

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 8, 2022

@vimalk78: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@vimalk78 vimalk78 mentioned this pull request May 9, 2022
Merged
@jcantrill
Copy link
Contributor

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 9, 2022
jcantrill
jcantrill reviewed May 9, 2022
View reviewed changes
test/helpers/types/types.go
@@ -31,7 +31,8 @@ type ContainerLog struct {
Hostname string `json:"hostname"`
PipelineMetadata PipelineMetadata `json:"pipeline_metadata"`
LogType string `json:"log_type,omitempty"`
ViaqIndexName string `json:"viaq_index_name"`
ViaqIndexName string `json:"viaq_index_name,omitempty"`
WriteIndex string `json:"write_index,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need "write" index?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the field we used for vector, as the name viaq-index-name didnt make much sense for vector.
Also vector ES sink does not delete the key which contains the index name, as fluentd ES plugin can do. So if we keep viaq-index-name , it will be added to the record also.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alanconway I defer to you here as this is a breaking viaq model change. I like "write-idex" better but its note currently in the model.

ES sink does not delete the key which contains the index name, as fluentd ES plugin can do
Is this field removed now? I thought I had seen it in the set of fields when I retrieve logs from ES

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alanconway any comments?

test/helpers/types/types.go
NamespaceName string `json:"namespace_name,omitempty"`
PodName string `json:"pod_name,omitempty"`
ContainerImage string `json:"container_image,omitempty"`
ContainerImageID string `json:"container_image_id,omitempty"`
PodNodeName string `json:"pod_node_name,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need pod node name? how is this different from "host"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pod_node_name contains value from spec.nodeName

Copy link
Contributor

@jcantrill jcantrill May 9, 2022 •
edited

Choose a reason for hiding this comment

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

pod_node_name contains value from spec.nodeName

spec.nodeName should be the same as the field host . I don't understand why it is different

test/framework/functional/vector/message_templates.go
@@ -0,0 +1,85 @@
package vector
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does vector require its own set of message templates if our goal is to adhere to the same Viaq data model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there are some differences in the data model, as captured in the google docs sheet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please point me to some specifics for further evaluation

alanconway
alanconway approved these changes May 13, 2022
View reviewed changes
Copy link
Contributor

@alanconway alanconway left a comment

Choose a reason for hiding this comment

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

LGTM but I'll let @jcantrill finish his deliberations before merging.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 13, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alanconway, vimalk78

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [alanconway,vimalk78]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vimalk78
Copy link
Contributor Author

Closing this in favor of #1466

@vimalk78 vimalk78 closed this May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcantrill jcantrill jcantrill left review comments

@alanconway alanconway alanconway approved these changes

@cahartma cahartma Awaiting requested review from cahartma

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@vimalk78 @jcantrill @alanconway